[PATCH 00/24] Fixes around device realization

[PATCH 00/24] Fixes around device realization

[Markus Armbruster]

This fixes a bunch of bugs I ran into while reworking how qdevs plug
into buses.  I instrumented the code a bit to flush out instances of
bug patterns.  I'll post these hacks separately.

Impact is less than clear in places.  Help with that is appreciated.

Markus Armbruster (24):
  arm/stm32f405: Fix realization of "stm32f2xx-adc" devices
  display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
  sd/pxa2xx_mmci: Fix to realize "pxa2xx-mmci" device
  aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices
  aspeed: Don't create unwanted "cortex-a7-arm-cpu" devices
  armv7m: Bury unwanted "ARM,bitband-memory" devices
  auxbus: Fix aux-to-i2c-bridge to be a subtype of aux-slave
  mac_via: Fix to realize "mos6522-q800-via*" devices
  macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices
  macio: Bury unwanted "macio-gpio" devices
  pnv/phb4: Bury unwanted "pnv-phb4-pec-stack" devices
  MAINTAINERS: Make section PowerNV cover pci-host/pnv* as well
  ppc4xx: Drop redundant device realization
  macio: Put "macio-nvram" device on the macio bus
  macio: Fix macio-bus to be a subtype of System bus
  ppc/pnv: Put "*-pnv-chip" and "pnv-xive" on the main system bus
  pnv/psi: Correct the pnv-psi* devices not to be sysbus devices
  display/sm501 display/ati: Fix to realize "i2c-ddc"
  riscv: Fix to put "riscv.hart_array" devices on sysbus
  riscv: Fix type of SiFive[EU]SocState, member parent_obj
  sparc/leon3: Fix to put grlib,* devices on sysbus
  qdev: Assert devices are plugged into a bus that can take them
  sd: Hide the qdev-but-not-quite thing created by sd_init()
  qdev: Assert onboard devices all get realized properly

 include/hw/ppc/pnv_psi.h    |  2 +-
 include/hw/riscv/sifive_e.h |  2 +-
 include/hw/riscv/sifive_u.h |  2 +-
 hw/arm/armv7m.c             |  6 ++++--
 hw/arm/aspeed_ast2600.c     |  5 ++++-
 hw/arm/aspeed_soc.c         |  2 +-
 hw/arm/stm32f405_soc.c      | 20 ++++++++++---------
 hw/core/qdev.c              | 21 +++++++++++++++++++
 hw/display/ati.c            |  1 +
 hw/display/sm501.c          |  1 +
 hw/display/xlnx_dp.c        |  4 ++++
 hw/misc/auxbus.c            |  2 +-
 hw/misc/mac_via.c           |  3 +++
 hw/misc/macio/cuda.c        |  8 +++-----
 hw/misc/macio/macio.c       |  7 +++++--
 hw/misc/macio/pmu.c         |  8 +++-----
 hw/pci-host/pnv_phb4_pec.c  |  3 +++
 hw/ppc/pnv.c                |  6 +++---
 hw/ppc/pnv_psi.c            |  2 +-
 hw/ppc/ppc440_uc.c          |  2 --
 hw/riscv/sifive_e.c         |  5 ++---
 hw/riscv/sifive_u.c         | 14 ++++++-------
 hw/riscv/spike.c            | 12 +++++------
 hw/riscv/virt.c             |  4 ++--
 hw/sd/pxa2xx_mmci.c         |  1 +
 hw/sd/sd.c                  | 40 ++++++++++++++++++++++++++-----------
 hw/sparc/leon3.c            |  4 ++--
 MAINTAINERS                 |  2 ++
 28 files changed, 121 insertions(+), 68 deletions(-)

-- 
2.21.1

[PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

[Markus Armbruster]

stm32f405_soc_initfn() creates six such devices, but
stm32f405_soc_realize() realizes only one.  Affects machine
netduinoplus2.

I wonder how this ever worked.  If the "device becomes real only on
realize" thing actually works, then we've always been missing five of
six such devices, yet nobody noticed.

Fix stm32f405_soc_realize() to realize all six.  Visible in "info
qtree":

     bus: main-system-bus
       type System
       dev: stm32f405-soc, id ""
         cpu-type = "cortex-m4-arm-cpu"
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
         mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: armv7m, id ""

The mmio addresses look suspicious.

Fixes: 529fc5fd3e18ace8f739afd02dc0953354f39442
Cc: Alistair Francis <alistair@alistair23.me>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/stm32f405_soc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
index 4f10ce6176..4649502711 100644
--- a/hw/arm/stm32f405_soc.c
+++ b/hw/arm/stm32f405_soc.c
@@ -185,16 +185,18 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
     qdev_connect_gpio_out(DEVICE(&s->adc_irqs), 0,
                           qdev_get_gpio_in(armv7m, ADC_IRQ));
 
-    dev = DEVICE(&(s->adc[i]));
-    object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
-    if (err != NULL) {
-        error_propagate(errp, err);
-        return;
+    for (i = 0; i < STM_NUM_ADCS; i++) {
+        dev = DEVICE(&(s->adc[i]));
+        object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
+        }
+        busdev = SYS_BUS_DEVICE(dev);
+        sysbus_mmio_map(busdev, 0, ADC_ADDR);
+        sysbus_connect_irq(busdev, 0,
+                           qdev_get_gpio_in(DEVICE(&s->adc_irqs), i));
     }
-    busdev = SYS_BUS_DEVICE(dev);
-    sysbus_mmio_map(busdev, 0, ADC_ADDR);
-    sysbus_connect_irq(busdev, 0,
-                       qdev_get_gpio_in(DEVICE(&s->adc_irqs), i));
 
     /* SPI devices */
     for (i = 0; i < STM_NUM_SPIS; i++) {
-- 
2.21.1

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

[Alistair Francis]

On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> stm32f405_soc_initfn() creates six such devices, but
> stm32f405_soc_realize() realizes only one.  Affects machine
> netduinoplus2.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing five of
> six such devices, yet nobody noticed.

I must have just been testing the first ADC.

>
> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
> qtree":
>
>      bus: main-system-bus
>        type System
>        dev: stm32f405-soc, id ""
>          cpu-type = "cortex-m4-arm-cpu"
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>          mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: armv7m, id ""
>
> The mmio addresses look suspicious.

Good catch, thanks :)

>
> Fixes: 529fc5fd3e18ace8f739afd02dc0953354f39442
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/stm32f405_soc.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
> index 4f10ce6176..4649502711 100644
> --- a/hw/arm/stm32f405_soc.c
> +++ b/hw/arm/stm32f405_soc.c
> @@ -185,16 +185,18 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
>      qdev_connect_gpio_out(DEVICE(&s->adc_irqs), 0,
>                            qdev_get_gpio_in(armv7m, ADC_IRQ));
>
> -    dev = DEVICE(&(s->adc[i]));
> -    object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
> -    if (err != NULL) {
> -        error_propagate(errp, err);
> -        return;
> +    for (i = 0; i < STM_NUM_ADCS; i++) {
> +        dev = DEVICE(&(s->adc[i]));
> +        object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        busdev = SYS_BUS_DEVICE(dev);
> +        sysbus_mmio_map(busdev, 0, ADC_ADDR);
> +        sysbus_connect_irq(busdev, 0,
> +                           qdev_get_gpio_in(DEVICE(&s->adc_irqs), i));
>      }
> -    busdev = SYS_BUS_DEVICE(dev);
> -    sysbus_mmio_map(busdev, 0, ADC_ADDR);
> -    sysbus_connect_irq(busdev, 0,
> -                       qdev_get_gpio_in(DEVICE(&s->adc_irqs), i));
>
>      /* SPI devices */
>      for (i = 0; i < STM_NUM_SPIS; i++) {
> --
> 2.21.1
>
>

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

[Markus Armbruster]

Alistair Francis <alistair23@gmail.com> writes:

> On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> stm32f405_soc_initfn() creates six such devices, but
>> stm32f405_soc_realize() realizes only one.  Affects machine
>> netduinoplus2.
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing five of
>> six such devices, yet nobody noticed.
>
> I must have just been testing the first ADC.
>
>>
>> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
>> qtree":
>>
>>      bus: main-system-bus
>>        type System
>>        dev: stm32f405-soc, id ""
>>          cpu-type = "cortex-m4-arm-cpu"
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>          mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: armv7m, id ""
>>
>> The mmio addresses look suspicious.
>
> Good catch, thanks :)

I'd love to squash in corrections, but I don't know the correct
addresses.  Can you help?

>>
>> Fixes: 529fc5fd3e18ace8f739afd02dc0953354f39442
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Thanks!

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

[Alistair Francis]

On Mon, May 18, 2020 at 10:08 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Alistair Francis <alistair23@gmail.com> writes:
>
> > On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> stm32f405_soc_initfn() creates six such devices, but
> >> stm32f405_soc_realize() realizes only one.  Affects machine
> >> netduinoplus2.
> >>
> >> I wonder how this ever worked.  If the "device becomes real only on
> >> realize" thing actually works, then we've always been missing five of
> >> six such devices, yet nobody noticed.
> >
> > I must have just been testing the first ADC.
> >
> >>
> >> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
> >> qtree":
> >>
> >>      bus: main-system-bus
> >>        type System
> >>        dev: stm32f405-soc, id ""
> >>          cpu-type = "cortex-m4-arm-cpu"
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>          mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: armv7m, id ""
> >>
> >> The mmio addresses look suspicious.
> >
> > Good catch, thanks :)
>
> I'd love to squash in corrections, but I don't know the correct
> addresses.  Can you help?

Yep, thanks for squashing it in.

The three addresses are:

0x40012000
0x40012100
0x40012200

and they all share interrupt number 18.

Let me know if you want me to do it.

Alistair

>
> >>
> >> Fixes: 529fc5fd3e18ace8f739afd02dc0953354f39442
> >> Cc: Alistair Francis <alistair@alistair23.me>
> >> Cc: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: qemu-arm@nongnu.org
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Thanks!
>

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

[Markus Armbruster]

Alistair Francis <alistair23@gmail.com> writes:

> On Mon, May 18, 2020 at 10:08 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Alistair Francis <alistair23@gmail.com> writes:
>>
>> > On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> stm32f405_soc_initfn() creates six such devices, but
>> >> stm32f405_soc_realize() realizes only one.  Affects machine
>> >> netduinoplus2.
>> >>
>> >> I wonder how this ever worked.  If the "device becomes real only on
>> >> realize" thing actually works, then we've always been missing five of
>> >> six such devices, yet nobody noticed.
>> >
>> > I must have just been testing the first ADC.
>> >
>> >>
>> >> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
>> >> qtree":
>> >>
>> >>      bus: main-system-bus
>> >>        type System
>> >>        dev: stm32f405-soc, id ""
>> >>          cpu-type = "cortex-m4-arm-cpu"
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>          mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: armv7m, id ""
>> >>
>> >> The mmio addresses look suspicious.
>> >
>> > Good catch, thanks :)
>>
>> I'd love to squash in corrections, but I don't know the correct
>> addresses.  Can you help?
>
> Yep, thanks for squashing it in.
>
> The three addresses are:
>
> 0x40012000
> 0x40012100
> 0x40012200
>
> and they all share interrupt number 18.

An the other three?  There are six devices in total...

> Let me know if you want me to do it.

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

[Philippe Mathieu-Daudé]

On 5/27/20 11:27 AM, Markus Armbruster wrote:
> Alistair Francis <alistair23@gmail.com> writes:
> 
>> On Mon, May 18, 2020 at 10:08 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Alistair Francis <alistair23@gmail.com> writes:
>>>
>>>> On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>>>
>>>>> stm32f405_soc_initfn() creates six such devices, but
>>>>> stm32f405_soc_realize() realizes only one.  Affects machine
>>>>> netduinoplus2.
>>>>>
>>>>> I wonder how this ever worked.  If the "device becomes real only on
>>>>> realize" thing actually works, then we've always been missing five of
>>>>> six such devices, yet nobody noticed.
>>>>
>>>> I must have just been testing the first ADC.
>>>>
>>>>>
>>>>> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
>>>>> qtree":
>>>>>
>>>>>      bus: main-system-bus
>>>>>        type System
>>>>>        dev: stm32f405-soc, id ""
>>>>>          cpu-type = "cortex-m4-arm-cpu"
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>          mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: armv7m, id ""
>>>>>
>>>>> The mmio addresses look suspicious.
>>>>
>>>> Good catch, thanks :)
>>>
>>> I'd love to squash in corrections, but I don't know the correct
>>> addresses.  Can you help?
>>
>> Yep, thanks for squashing it in.
>>
>> The three addresses are:
>>
>> 0x40012000
>> 0x40012100
>> 0x40012200
>>
>> and they all share interrupt number 18.
> 
> An the other three?  There are six devices in total...

Alistair looked at the stm32f205, which has 3 ADCs (and seems correct).

> 
>> Let me know if you want me to do it.

Alistair, the problem is the stm32f405. I guess the bug come from
copy/pasting then modifying for the shared IRQ.

    /* Timer 2 to 5 */
    for (i = 0; i < STM_NUM_TIMERS; i++) {
       ...
    }
    ...
    /* ADC device, the IRQs are ORed together */
    ...
    dev = DEVICE(&(s->adc[i])); <=== i = STM_NUM_TIMERS = 4
    object_property_set_bool(OBJECT(&s->adc[i]),
                             true, "realized", &err);

Only ADC#3 is realized, then mapped at 0x40012000.

You need to unparent the others anyway, so it is easier to realize
adc[0] and unparent the rest:

    for (i = 1; i < STM_NUM_ADCS; i++) {
       ...
    }

[PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Markus Armbruster]

xlnx_dp_init() creates these two devices, but they're never realized.
Affects machine xlnx-zcu102.

I wonder how this ever worked.  If the "device becomes real only on
realize" thing actually works, then we've always been missing these
two devices, yet nobody noticed.

Fix by realizing them in xlnx_dp_realize().

Fixes: 58ac482a66de09a7590f705e53fc6a3fb8a055e8
Cc: KONRAD Frederic <fred.konrad@greensocs.com>
Cc: Alistair Francis <alistair@alistair23.me>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/display/xlnx_dp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 3e5fb44e06..bdc229a51e 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1264,9 +1264,13 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
     DisplaySurface *surface;
     struct audsettings as;
 
+    qdev_init_nofail(DEVICE(s->aux_bus->bridge));
+
     qdev_init_nofail(DEVICE(s->dpcd));
     aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
 
+    qdev_init_nofail(DEVICE(s->edid));
+
     s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
     surface = qemu_console_surface(s->console);
     xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL,
-- 
2.21.1

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Fred Konrad]

Le 5/18/20 à 7:03 AM, Markus Armbruster a écrit :
> xlnx_dp_init() creates these two devices, but they're never realized.
> Affects machine xlnx-zcu102.
> 
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing these
> two devices, yet nobody noticed.

I can't tell, but it used to work back in 2016 since these devices were required
to have a working framebuffer.

> 
> Fix by realizing them in xlnx_dp_realize().
> 
> Fixes: 58ac482a66de09a7590f705e53fc6a3fb8a055e8
> Cc: KONRAD Frederic <fred.konrad@greensocs.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/display/xlnx_dp.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 3e5fb44e06..bdc229a51e 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1264,9 +1264,13 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>       DisplaySurface *surface;
>       struct audsettings as;
>   
> +    qdev_init_nofail(DEVICE(s->aux_bus->bridge));
> +
>       qdev_init_nofail(DEVICE(s->dpcd));
>       aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>   
> +    qdev_init_nofail(DEVICE(s->edid));
> +
>       s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
>       surface = qemu_console_surface(s->console);
>       xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL,
> 

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Markus Armbruster]

Fred Konrad <konrad@adacore.com> writes:

> Le 5/18/20 à 7:03 AM, Markus Armbruster a écrit :
>> xlnx_dp_init() creates these two devices, but they're never realized.
>> Affects machine xlnx-zcu102.
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing these
>> two devices, yet nobody noticed.
>
> I can't tell, but it used to work back in 2016 since these devices were required
> to have a working framebuffer.

I don't doubt you.

I figure the "device becomes real only on realize" thing is actually
more myth than thing.

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Peter Maydell]

On Tue, 19 May 2020 at 06:09, Markus Armbruster <armbru@redhat.com> wrote:
> I figure the "device becomes real only on realize" thing is actually
> more myth than thing.

It's not a myth, it's an API guarantee thing. If you don't realize
the device you create before you use it then you're in the world of
unspecified behaviour, and anything could happen: maybe it works,
maybe it doesn't, maybe it works today and breaks tomorrow.

thanks
-- PMM

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 19 May 2020 at 06:09, Markus Armbruster <armbru@redhat.com> wrote:
>> I figure the "device becomes real only on realize" thing is actually
>> more myth than thing.
>
> It's not a myth, it's an API guarantee thing. If you don't realize
> the device you create before you use it then you're in the world of
> unspecified behaviour, and anything could happen: maybe it works,
> maybe it doesn't, maybe it works today and breaks tomorrow.

It's a myth in the sense "we want it to be that way, but it often
ain't" :)

Of course your right in that it is also a case of "use the interface the
specified way, or else".

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Edgar E. Iglesias]

On Mon, May 18, 2020 at 07:03:46AM +0200, Markus Armbruster wrote:
> xlnx_dp_init() creates these two devices, but they're never realized.
> Affects machine xlnx-zcu102.
> 
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing these
> two devices, yet nobody noticed.
> 
> Fix by realizing them in xlnx_dp_realize().

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Fixes: 58ac482a66de09a7590f705e53fc6a3fb8a055e8
> Cc: KONRAD Frederic <fred.konrad@greensocs.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/display/xlnx_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 3e5fb44e06..bdc229a51e 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1264,9 +1264,13 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>      DisplaySurface *surface;
>      struct audsettings as;
>  
> +    qdev_init_nofail(DEVICE(s->aux_bus->bridge));
> +
>      qdev_init_nofail(DEVICE(s->dpcd));
>      aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>  
> +    qdev_init_nofail(DEVICE(s->edid));
> +
>      s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
>      surface = qemu_console_surface(s->console);
>      xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL,
> -- 
> 2.21.1
> 

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Peter Maydell]

On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
>
> xlnx_dp_init() creates these two devices, but they're never realized.
> Affects machine xlnx-zcu102.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing these
> two devices, yet nobody noticed.

It depends entirely on the implementation of the device.
If it happens to do nothing in the realize method that
matters (eg i2c-ddc has no realize method and does the limited
amount of initialization it needs in instance_init) then the
device will (by lucky accident) work just fine.

We should really ideally have an assert() in the DeviceClass
reset that the device was realized, so we can keep this kind
of bug out of the codebase. (Last time I looked it wasn't obvious
exactly where to put the assert now that we have both legacy-reset
and three-phase-reset, unfortunately.)

thanks
-- PMM

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Philippe Mathieu-Daudé]

On 5/18/20 12:30 PM, Peter Maydell wrote:
> On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> xlnx_dp_init() creates these two devices, but they're never realized.
>> Affects machine xlnx-zcu102.
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing these
>> two devices, yet nobody noticed.
> 
> It depends entirely on the implementation of the device.
> If it happens to do nothing in the realize method that
> matters (eg i2c-ddc has no realize method and does the limited
> amount of initialization it needs in instance_init) then the
> device will (by lucky accident) work just fine.
> 
> We should really ideally have an assert() in the DeviceClass
> reset that the device was realized, so we can keep this kind
> of bug out of the codebase. (Last time I looked it wasn't obvious
> exactly where to put the assert now that we have both legacy-reset
> and three-phase-reset, unfortunately.)

Your wish came true in the last patch of this series! #24:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg704239.html

> 
> thanks
> -- PMM
> 

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 5/18/20 12:30 PM, Peter Maydell wrote:
>> On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> xlnx_dp_init() creates these two devices, but they're never realized.
>>> Affects machine xlnx-zcu102.
>>>
>>> I wonder how this ever worked.  If the "device becomes real only on
>>> realize" thing actually works, then we've always been missing these
>>> two devices, yet nobody noticed.
>>
>> It depends entirely on the implementation of the device.
>> If it happens to do nothing in the realize method that
>> matters

Also in the realize methods of supertypes.

>>         (eg i2c-ddc has no realize method and does the limited
>> amount of initialization it needs in instance_init) then the
>> device will (by lucky accident) work just fine.

Yes.

>> We should really ideally have an assert() in the DeviceClass
>> reset that the device was realized, so we can keep this kind
>> of bug out of the codebase. (Last time I looked it wasn't obvious
>> exactly where to put the assert now that we have both legacy-reset
>> and three-phase-reset, unfortunately.)
>
> Your wish came true in the last patch of this series! #24:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg704239.html

Not exactly what Peter asked for, but hopefully close enough for
practical purposes.

Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Alistair Francis]

On Sun, May 17, 2020 at 10:14 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> xlnx_dp_init() creates these two devices, but they're never realized.
> Affects machine xlnx-zcu102.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing these
> two devices, yet nobody noticed.
>
> Fix by realizing them in xlnx_dp_realize().
>
> Fixes: 58ac482a66de09a7590f705e53fc6a3fb8a055e8
> Cc: KONRAD Frederic <fred.konrad@greensocs.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/xlnx_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 3e5fb44e06..bdc229a51e 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1264,9 +1264,13 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>      DisplaySurface *surface;
>      struct audsettings as;
>
> +    qdev_init_nofail(DEVICE(s->aux_bus->bridge));
> +
>      qdev_init_nofail(DEVICE(s->dpcd));
>      aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>
> +    qdev_init_nofail(DEVICE(s->edid));
> +
>      s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
>      surface = qemu_console_surface(s->console);
>      xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL,
> --
> 2.21.1
>
>

[PATCH 03/24] sd/pxa2xx_mmci: Fix to realize "pxa2xx-mmci" device

[Markus Armbruster]

pxa2xx_mmci_init() creates a "pxa2xx-mmci" device, but neglects to
realize it.  Affects machines akita, borzoi, connex, mainstone, spitz,
terrier, tosa, verdex, and z2.

I wonder how this ever worked.  If the "device becomes real only on
realize" thing actually works, then we've always been missing the
device, yet nobody noticed.

Fix by realizing it right away.  Visible in "info qom-tree"; here's
the change for akita:

     /machine (akita-machine)
       [...]
       /unattached (container)
         [...]
    +    /device[5] (pxa2xx-mmci)
    +      /pxa2xx-mmci[0] (qemu:memory-region)
    +      /sd-bus (pxa2xx-mmci-bus)
         [rest of device[*] renumbered...]

Fixes: 7a9468c92517e19037bfe2272f64f5dadaf9db15
Cc: Andrzej Zaborowski <balrogg@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sd/pxa2xx_mmci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 8f9ab0ec16..6a80181bc9 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -492,6 +492,7 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
     sysbus_connect_irq(sbd, 0, irq);
     qdev_connect_gpio_out_named(dev, "rx-dma", 0, rx_dma);
     qdev_connect_gpio_out_named(dev, "tx-dma", 0, tx_dma);
+    qdev_init_nofail(dev);
 
     /* Create and plug in the sd card */
     carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"), TYPE_SD_CARD);
-- 
2.21.1

Re: [PATCH 03/24] sd/pxa2xx_mmci: Fix to realize "pxa2xx-mmci" device

[Peter Maydell]

On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
>
> pxa2xx_mmci_init() creates a "pxa2xx-mmci" device, but neglects to
> realize it.  Affects machines akita, borzoi, connex, mainstone, spitz,
> terrier, tosa, verdex, and z2.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing the
> device, yet nobody noticed.

It works by accident: because the device in question happens
to not have a realize method, nothing breaks if we forget
to run the realize method. Undefined behaviour: we happened
to get lucky in this case.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Markus Armbruster]

These devices are optional, and controlled by @nb_nics.
aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
supported number.  aspeed_soc_ast2600_realize() and
aspeed_soc_realize() realize only the wanted number.  Works, although
it can leave unrealized devices hanging around in the QOM composition
tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.

Make the init functions create only the wanted ones.  Visible in "info
qom-tree"; here's the change for ast2600-evb:

     /machine (ast2600-evb-machine)
       [...]
       /soc (ast2600-a1)
         [...]
         /ftgmac100[0] (ftgmac100)
           /ftgmac100[0] (qemu:memory-region)
    -    /ftgmac100[1] (ftgmac100)
    -    /ftgmac100[2] (ftgmac100)
    -    /ftgmac100[3] (ftgmac100)
         /gpio (aspeed.gpio-ast2600)
         [...]
         /mii[0] (aspeed-mmi)
           /aspeed-mmi[0] (qemu:memory-region)
    -    /mii[1] (aspeed-mmi)
    -    /mii[2] (aspeed-mmi)
    -    /mii[3] (aspeed-mmi)
         /rtc (aspeed.rtc)

I'm not sure creating @nb_nics devices makes sense.  How many does the
physical chip provide?

Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Joel Stanley <joel@jms.id.au>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/aspeed_ast2600.c | 2 +-
 hw/arm/aspeed_soc.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 71a0acfe26..0a6a77dd54 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -188,7 +188,7 @@ static void aspeed_soc_ast2600_init(Object *obj)
                               sizeof(s->wdt[i]), typename);
     }
 
-    for (i = 0; i < sc->macs_num; i++) {
+    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
         sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(&s->ftgmac100[i]),
                               sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
 
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index cf6b6dd116..7ca860392a 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -203,7 +203,7 @@ static void aspeed_soc_init(Object *obj)
                               sizeof(s->wdt[i]), typename);
     }
 
-    for (i = 0; i < sc->macs_num; i++) {
+    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
         sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(&s->ftgmac100[i]),
                               sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
     }
-- 
2.21.1

Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Cédric Le Goater]

On 5/18/20 7:03 AM, Markus Armbruster wrote:
> These devices are optional, and controlled by @nb_nics.
> aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
> supported number.  aspeed_soc_ast2600_realize() and
> aspeed_soc_realize() realize only the wanted number.  Works, although
> it can leave unrealized devices hanging around in the QOM composition
> tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
> romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
> 
> Make the init functions create only the wanted ones.  Visible in "info
> qom-tree"; here's the change for ast2600-evb:
> 
>      /machine (ast2600-evb-machine)
>        [...]
>        /soc (ast2600-a1)
>          [...]
>          /ftgmac100[0] (ftgmac100)
>            /ftgmac100[0] (qemu:memory-region)
>     -    /ftgmac100[1] (ftgmac100)
>     -    /ftgmac100[2] (ftgmac100)
>     -    /ftgmac100[3] (ftgmac100)
>          /gpio (aspeed.gpio-ast2600)
>          [...]
>          /mii[0] (aspeed-mmi)
>            /aspeed-mmi[0] (qemu:memory-region)
>     -    /mii[1] (aspeed-mmi)
>     -    /mii[2] (aspeed-mmi)
>     -    /mii[3] (aspeed-mmi)
>          /rtc (aspeed.rtc)
> 
> I'm not sure creating @nb_nics devices makes sense.  How many does the
> physical chip provide?

The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
define the one it uses, generally MAC0 but the tacoma board uses MAC3.

Shouldn't the model reflect the real address space independently from
the NIC backends defined on the command line ?  

How should we proceed in such cases ? 

Thanks,

C. 

> 
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/aspeed_ast2600.c | 2 +-
>  hw/arm/aspeed_soc.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 71a0acfe26..0a6a77dd54 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -188,7 +188,7 @@ static void aspeed_soc_ast2600_init(Object *obj)
>                                sizeof(s->wdt[i]), typename);
>      }
>  
> -    for (i = 0; i < sc->macs_num; i++) {
> +    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
>          sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(&s->ftgmac100[i]),
>                                sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
>  
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index cf6b6dd116..7ca860392a 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -203,7 +203,7 @@ static void aspeed_soc_init(Object *obj)
>                                sizeof(s->wdt[i]), typename);
>      }
>  
> -    for (i = 0; i < sc->macs_num; i++) {
> +    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
>          sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(&s->ftgmac100[i]),
>                                sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
>      }
> 

Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Andrew Jeffery]

On Mon, 18 May 2020, at 21:49, Cédric Le Goater wrote:
> On 5/18/20 7:03 AM, Markus Armbruster wrote:
> > These devices are optional, and controlled by @nb_nics.
> > aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
> > supported number.  aspeed_soc_ast2600_realize() and
> > aspeed_soc_realize() realize only the wanted number.  Works, although
> > it can leave unrealized devices hanging around in the QOM composition
> > tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
> > romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
> > 
> > Make the init functions create only the wanted ones.  Visible in "info
> > qom-tree"; here's the change for ast2600-evb:
> > 
> >      /machine (ast2600-evb-machine)
> >        [...]
> >        /soc (ast2600-a1)
> >          [...]
> >          /ftgmac100[0] (ftgmac100)
> >            /ftgmac100[0] (qemu:memory-region)
> >     -    /ftgmac100[1] (ftgmac100)
> >     -    /ftgmac100[2] (ftgmac100)
> >     -    /ftgmac100[3] (ftgmac100)
> >          /gpio (aspeed.gpio-ast2600)
> >          [...]
> >          /mii[0] (aspeed-mmi)
> >            /aspeed-mmi[0] (qemu:memory-region)
> >     -    /mii[1] (aspeed-mmi)
> >     -    /mii[2] (aspeed-mmi)
> >     -    /mii[3] (aspeed-mmi)
> >          /rtc (aspeed.rtc)
> > 
> > I'm not sure creating @nb_nics devices makes sense.  How many does the
> > physical chip provide?
> 
> The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
> define the one it uses, generally MAC0 but the tacoma board uses MAC3.
> 
> Shouldn't the model reflect the real address space independently from
> the NIC backends defined on the command line ?  

That's my feeling too, though I'm not sure what to make of the unrealised devices
in the QOM tree. Does it matter? It hasn't bothered me.

Andrew

Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Markus Armbruster]

"Andrew Jeffery" <andrew@aj.id.au> writes:

> On Mon, 18 May 2020, at 21:49, Cédric Le Goater wrote:
>> On 5/18/20 7:03 AM, Markus Armbruster wrote:
>> > These devices are optional, and controlled by @nb_nics.
>> > aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
>> > supported number.  aspeed_soc_ast2600_realize() and
>> > aspeed_soc_realize() realize only the wanted number.  Works, although
>> > it can leave unrealized devices hanging around in the QOM composition
>> > tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
>> > romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
>> > 
>> > Make the init functions create only the wanted ones.  Visible in "info
>> > qom-tree"; here's the change for ast2600-evb:
>> > 
>> >      /machine (ast2600-evb-machine)
>> >        [...]
>> >        /soc (ast2600-a1)
>> >          [...]
>> >          /ftgmac100[0] (ftgmac100)
>> >            /ftgmac100[0] (qemu:memory-region)
>> >     -    /ftgmac100[1] (ftgmac100)
>> >     -    /ftgmac100[2] (ftgmac100)
>> >     -    /ftgmac100[3] (ftgmac100)
>> >          /gpio (aspeed.gpio-ast2600)
>> >          [...]
>> >          /mii[0] (aspeed-mmi)
>> >            /aspeed-mmi[0] (qemu:memory-region)
>> >     -    /mii[1] (aspeed-mmi)
>> >     -    /mii[2] (aspeed-mmi)
>> >     -    /mii[3] (aspeed-mmi)
>> >          /rtc (aspeed.rtc)
>> > 
>> > I'm not sure creating @nb_nics devices makes sense.  How many does the
>> > physical chip provide?
>> 
>> The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
>> define the one it uses, generally MAC0 but the tacoma board uses MAC3.
>> 
>> Shouldn't the model reflect the real address space independently from
>> the NIC backends defined on the command line ?  
>
> That's my feeling too, though I'm not sure what to make of the unrealised devices
> in the QOM tree. Does it matter? It hasn't bothered me.

Depending on what the initialization code does, unrealized devices can
be anything from a little wasted memory to open bear trap.  I don't
really expect the latter extreme in the code, as I expect bear traps to
quickly catch the developer that set them.

I guess the unrealized devices cleaned up in this patch did no actual
harm.

Still, it's an unhealthy state, and that's why I clean it up.  "[PATCH
24/24] qdev: Assert onboard devices all get realized properly" should
ensure we stay clean.

Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Philippe Mathieu-Daudé]

On 5/19/20 7:45 AM, Markus Armbruster wrote:
> "Andrew Jeffery" <andrew@aj.id.au> writes:
> 
>> On Mon, 18 May 2020, at 21:49, Cédric Le Goater wrote:
>>> On 5/18/20 7:03 AM, Markus Armbruster wrote:
>>>> These devices are optional, and controlled by @nb_nics.
>>>> aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
>>>> supported number.  aspeed_soc_ast2600_realize() and
>>>> aspeed_soc_realize() realize only the wanted number.  Works, although
>>>> it can leave unrealized devices hanging around in the QOM composition
>>>> tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
>>>> romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
>>>>
>>>> Make the init functions create only the wanted ones.  Visible in "info
>>>> qom-tree"; here's the change for ast2600-evb:
>>>>
>>>>       /machine (ast2600-evb-machine)
>>>>         [...]
>>>>         /soc (ast2600-a1)
>>>>           [...]
>>>>           /ftgmac100[0] (ftgmac100)
>>>>             /ftgmac100[0] (qemu:memory-region)
>>>>      -    /ftgmac100[1] (ftgmac100)
>>>>      -    /ftgmac100[2] (ftgmac100)
>>>>      -    /ftgmac100[3] (ftgmac100)
>>>>           /gpio (aspeed.gpio-ast2600)
>>>>           [...]
>>>>           /mii[0] (aspeed-mmi)
>>>>             /aspeed-mmi[0] (qemu:memory-region)
>>>>      -    /mii[1] (aspeed-mmi)
>>>>      -    /mii[2] (aspeed-mmi)
>>>>      -    /mii[3] (aspeed-mmi)
>>>>           /rtc (aspeed.rtc)
>>>>
>>>> I'm not sure creating @nb_nics devices makes sense.  How many does the
>>>> physical chip provide?
>>>
>>> The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
>>> define the one it uses, generally MAC0 but the tacoma board uses MAC3.
>>>
>>> Shouldn't the model reflect the real address space independently from
>>> the NIC backends defined on the command line ?

If the SoC has N ftgmac100 peripherals, you need to mmio-map the N 
instances, else your guest will get MEMTX_DECODE_ERROR trying to access 
it, regardless command line NIC plugged.

>>
>> That's my feeling too, though I'm not sure what to make of the unrealised devices
>> in the QOM tree. Does it matter? It hasn't bothered me.
> 
> Depending on what the initialization code does, unrealized devices can
> be anything from a little wasted memory to open bear trap.  I don't
> really expect the latter extreme in the code, as I expect bear traps to
> quickly catch the developer that set them.
> 
> I guess the unrealized devices cleaned up in this patch did no actual
> harm.
> 
> Still, it's an unhealthy state, and that's why I clean it up.  "[PATCH
> 24/24] qdev: Assert onboard devices all get realized properly" should
> ensure we stay clean.
> 
> 

Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Cédric Le Goater]

On 5/19/20 1:42 PM, Philippe Mathieu-Daudé wrote:
> On 5/19/20 7:45 AM, Markus Armbruster wrote:
>> "Andrew Jeffery" <andrew@aj.id.au> writes:
>>
>>> On Mon, 18 May 2020, at 21:49, Cédric Le Goater wrote:
>>>> On 5/18/20 7:03 AM, Markus Armbruster wrote:
>>>>> These devices are optional, and controlled by @nb_nics.
>>>>> aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
>>>>> supported number.  aspeed_soc_ast2600_realize() and
>>>>> aspeed_soc_realize() realize only the wanted number.  Works, although
>>>>> it can leave unrealized devices hanging around in the QOM composition
>>>>> tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
>>>>> romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
>>>>>
>>>>> Make the init functions create only the wanted ones.  Visible in "info
>>>>> qom-tree"; here's the change for ast2600-evb:
>>>>>
>>>>>       /machine (ast2600-evb-machine)
>>>>>         [...]
>>>>>         /soc (ast2600-a1)
>>>>>           [...]
>>>>>           /ftgmac100[0] (ftgmac100)
>>>>>             /ftgmac100[0] (qemu:memory-region)
>>>>>      -    /ftgmac100[1] (ftgmac100)
>>>>>      -    /ftgmac100[2] (ftgmac100)
>>>>>      -    /ftgmac100[3] (ftgmac100)
>>>>>           /gpio (aspeed.gpio-ast2600)
>>>>>           [...]
>>>>>           /mii[0] (aspeed-mmi)
>>>>>             /aspeed-mmi[0] (qemu:memory-region)
>>>>>      -    /mii[1] (aspeed-mmi)
>>>>>      -    /mii[2] (aspeed-mmi)
>>>>>      -    /mii[3] (aspeed-mmi)
>>>>>           /rtc (aspeed.rtc)
>>>>>
>>>>> I'm not sure creating @nb_nics devices makes sense.  How many does the
>>>>> physical chip provide?
>>>>
>>>> The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
>>>> define the one it uses, generally MAC0 but the tacoma board uses MAC3.
>>>>
>>>> Shouldn't the model reflect the real address space independently from
>>>> the NIC backends defined on the command line ?
> 
> If the SoC has N ftgmac100 peripherals, you need to mmio-map the N instances, 
> else your guest will get MEMTX_DECODE_ERROR trying to access it, regardless 
> command line NIC plugged.
 
yes. This is what I do with the patch attached below but I have another 
problem.

Get a witherspoon-tacoma flash image :

    https://openpower.xyz/job/openbmc-build/distro=ubuntu,label=builder,target=witherspoon-tacoma/lastSuccessfulBuild/artifact/deploy/images/witherspoon-tacoma/flash-witherspoon-tacoma

Run :

    qemu-system-arm -M tacoma-bmc -nic user -drive file=./flash-witherspoon-tacoma,format=raw,if=mtd -nographic -nodefaults -serial mon:stdio
    qemu-system-arm: warning: nic ftgmac100.0 has no peer
    qemu-system-arm: warning: nic ftgmac100.1 has no peer
    qemu-system-arm: warning: nic ftgmac100.3 has no peer
    
    
    U-Boot 2019.04 (May 06 2020 - 04:20:01 +0000)
    
    SOC: AST2600-A1 
    LPC Mode: SIO:Enable : SuperIO-2e
    Eth: MAC0: RMII/NCSI, MAC1: RMII/NCSI, MAC2: RMII/NCSI, MAC3: RMII/NCSI
    Model: Tacoma
    DRAM:  already initialized, 1008 MiB
    ...

How do I deal with the "no peer" warnings ? 

Thanks,

C.


From a3c2772eca8a541158345e6f219ce524f1bc017b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Tue, 19 May 2020 14:39:55 +0200
Subject: [PATCH] arm/aspeed: Rework NIC attachment
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed.h     |  1 +
 include/hw/arm/aspeed_soc.h |  1 +
 hw/arm/aspeed.c             |  5 +++++
 hw/arm/aspeed_ast2600.c     |  9 +++++++--
 hw/arm/aspeed_soc.c         | 10 ++++++++--
 5 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 18521484b90e..7e71152b3554 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -39,6 +39,7 @@ typedef struct AspeedMachineClass {
     const char *fmc_model;
     const char *spi_model;
     uint32_t num_cs;
+    uint32_t nic_mask;
     void (*i2c_init)(AspeedBoardState *bmc);
 } AspeedMachineClass;
 
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 914115f3ef77..32e9a232a049 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -55,6 +55,7 @@ typedef struct AspeedSoCState {
     AspeedSDMCState sdmc;
     AspeedWDTState wdt[ASPEED_WDTS_NUM];
     FTGMAC100State ftgmac100[ASPEED_MACS_NUM];
+    uint32_t nic_mask;
     AspeedMiiState mii[ASPEED_MACS_NUM];
     AspeedGPIOState gpio;
     AspeedGPIOState gpio_1_8v;
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6f8f4b88f8ab..338b5db20cf9 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -283,6 +283,8 @@ static void aspeed_machine_init(MachineState *machine)
                             &error_abort);
     object_property_set_int(OBJECT(&bmc->soc), amc->num_cs, "num-cs",
                             &error_abort);
+    object_property_set_int(OBJECT(&bmc->soc), amc->nic_mask, "nic-mask",
+                            &error_abort);
     object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container),
                              "dram", &error_abort);
     if (machine->kernel_filename) {
@@ -556,12 +558,14 @@ static int aspeed_soc_num_cpus(const char *soc_name)
 static void aspeed_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
 
     mc->init = aspeed_machine_init;
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
     mc->default_ram_id = "ram";
+    amc->nic_mask = 1; /* First NIC */
 
     aspeed_machine_class_props_init(oc);
 }
@@ -698,6 +702,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
     amc->fmc_model = "mx66l1g45g";
     amc->spi_model = "mx66l1g45g";
     amc->num_cs    = 2;
+    amc->nic_mask  = 0x4; /* third NIC */
     amc->i2c_init  = witherspoon_bmc_i2c_init; /* Same board layout */
     mc->default_ram_size = 1 * GiB;
     mc->default_cpus = mc->min_cpus = mc->max_cpus =
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 114b94f8f44d..0e303c3018e7 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -247,6 +247,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
     Error *err = NULL, *local_err = NULL;
     qemu_irq irq;
+    NICInfo *nd = &nd_table[0];
 
     /* IO space */
     create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
@@ -462,8 +463,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     }
 
     /* Net */
-    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
-        qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]);
+    for (i = 0; i < sc->macs_num; i++) {
+        if ((s->nic_mask & (1 << i)) && nd->used) {
+            qemu_check_nic_model(nd, TYPE_FTGMAC100);
+            qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd);
+            nd++;
+        }
         object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
                                  &err);
         object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized",
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 984d29087dce..cc4f6769e763 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -234,6 +234,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     AspeedSoCState *s = ASPEED_SOC(dev);
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
     Error *err = NULL, *local_err = NULL;
+    NICInfo *nd = &nd_table[0];
 
     /* IO space */
     create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
@@ -405,8 +406,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
 
     /* Net */
-    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
-        qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), &nd_table[i]);
+    for (i = 0; i < sc->macs_num; i++) {
+        if ((s->nic_mask & (1 << i)) && nd->used) {
+            qemu_check_nic_model(nd, TYPE_FTGMAC100);
+            qdev_set_nic_properties(DEVICE(&s->ftgmac100[i]), nd);
+            nd++;
+        }
         object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
                                  &err);
         object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized",
@@ -455,6 +460,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                        aspeed_soc_get_irq(s, ASPEED_SDHCI));
 }
 static Property aspeed_soc_properties[] = {
+    DEFINE_PROP_UINT32("nic-mask", AspeedSoCState, nic_mask, 0x1),
     DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
-- 
2.25.4

Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Joel Stanley]

On Mon, 18 May 2020 at 12:19, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 5/18/20 7:03 AM, Markus Armbruster wrote:
> > These devices are optional, and controlled by @nb_nics.
> > aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
> > supported number.  aspeed_soc_ast2600_realize() and
> > aspeed_soc_realize() realize only the wanted number.  Works, although
> > it can leave unrealized devices hanging around in the QOM composition
> > tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
> > romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
> >
> > Make the init functions create only the wanted ones.  Visible in "info
> > qom-tree"; here's the change for ast2600-evb:
> >
> >      /machine (ast2600-evb-machine)
> >        [...]
> >        /soc (ast2600-a1)
> >          [...]
> >          /ftgmac100[0] (ftgmac100)
> >            /ftgmac100[0] (qemu:memory-region)
> >     -    /ftgmac100[1] (ftgmac100)
> >     -    /ftgmac100[2] (ftgmac100)
> >     -    /ftgmac100[3] (ftgmac100)
> >          /gpio (aspeed.gpio-ast2600)
> >          [...]
> >          /mii[0] (aspeed-mmi)
> >            /aspeed-mmi[0] (qemu:memory-region)
> >     -    /mii[1] (aspeed-mmi)
> >     -    /mii[2] (aspeed-mmi)
> >     -    /mii[3] (aspeed-mmi)
> >          /rtc (aspeed.rtc)
> >
> > I'm not sure creating @nb_nics devices makes sense.  How many does the
> > physical chip provide?
>
> The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
> define the one it uses, generally MAC0 but the tacoma board uses MAC3.
>
> Shouldn't the model reflect the real address space independently from
> the NIC backends defined on the command line ?

Agreed, the MAC hardware is present in all instances of the AST2600,
so they should be present in qemu. Only some boards wire up a network
device to the other side.

It would be advantageous for us to be able to specify which device is
being connected to on the command line. Currently we do this by
connecting all devices up to the one we care about which is an ugly
workaround.

> How should we proceed in such cases ?
>
> Thanks,
>
> C.
>
> >
> > Cc: "Cédric Le Goater" <clg@kaod.org>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Andrew Jeffery <andrew@aj.id.au>
> > Cc: Joel Stanley <joel@jms.id.au>
> > Cc: qemu-arm@nongnu.org
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  hw/arm/aspeed_ast2600.c | 2 +-
> >  hw/arm/aspeed_soc.c     | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index 71a0acfe26..0a6a77dd54 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -188,7 +188,7 @@ static void aspeed_soc_ast2600_init(Object *obj)
> >                                sizeof(s->wdt[i]), typename);
> >      }
> >
> > -    for (i = 0; i < sc->macs_num; i++) {
> > +    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
> >          sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(&s->ftgmac100[i]),
> >                                sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
> >
> > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> > index cf6b6dd116..7ca860392a 100644
> > --- a/hw/arm/aspeed_soc.c
> > +++ b/hw/arm/aspeed_soc.c
> > @@ -203,7 +203,7 @@ static void aspeed_soc_init(Object *obj)
> >                                sizeof(s->wdt[i]), typename);
> >      }
> >
> > -    for (i = 0; i < sc->macs_num; i++) {
> > +    for (i = 0; i < nb_nics && i < sc->macs_num; i++) {
> >          sysbus_init_child_obj(obj, "ftgmac100[*]", OBJECT(&s->ftgmac100[i]),
> >                                sizeof(s->ftgmac100[i]), TYPE_FTGMAC100);
> >      }
> >
>

Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices

[Markus Armbruster]

Joel Stanley <joel@jms.id.au> writes:

> On Mon, 18 May 2020 at 12:19, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 5/18/20 7:03 AM, Markus Armbruster wrote:
>> > These devices are optional, and controlled by @nb_nics.
>> > aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
>> > supported number.  aspeed_soc_ast2600_realize() and
>> > aspeed_soc_realize() realize only the wanted number.  Works, although
>> > it can leave unrealized devices hanging around in the QOM composition
>> > tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
>> > romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
>> >
>> > Make the init functions create only the wanted ones.  Visible in "info
>> > qom-tree"; here's the change for ast2600-evb:
>> >
>> >      /machine (ast2600-evb-machine)
>> >        [...]
>> >        /soc (ast2600-a1)
>> >          [...]
>> >          /ftgmac100[0] (ftgmac100)
>> >            /ftgmac100[0] (qemu:memory-region)
>> >     -    /ftgmac100[1] (ftgmac100)
>> >     -    /ftgmac100[2] (ftgmac100)
>> >     -    /ftgmac100[3] (ftgmac100)
>> >          /gpio (aspeed.gpio-ast2600)
>> >          [...]
>> >          /mii[0] (aspeed-mmi)
>> >            /aspeed-mmi[0] (qemu:memory-region)
>> >     -    /mii[1] (aspeed-mmi)
>> >     -    /mii[2] (aspeed-mmi)
>> >     -    /mii[3] (aspeed-mmi)
>> >          /rtc (aspeed.rtc)
>> >
>> > I'm not sure creating @nb_nics devices makes sense.  How many does the
>> > physical chip provide?
>>
>> The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
>> define the one it uses, generally MAC0 but the tacoma board uses MAC3.
>>
>> Shouldn't the model reflect the real address space independently from
>> the NIC backends defined on the command line ?
>
> Agreed, the MAC hardware is present in all instances of the AST2600,
> so they should be present in qemu. Only some boards wire up a network
> device to the other side.

I guess an unwired NIC behaves as if no cable was plugged into the
external connector ("no carrier").

We can model that.

> It would be advantageous for us to be able to specify which device is
> being connected to on the command line. Currently we do this by
> connecting all devices up to the one we care about which is an ugly
> workaround.

We use -nic to configure onboard NICs.

The configuration gets deposited in nd_table[] for board code to pick
up.

Boards use nd_table[0] for their first NIC, nd_table[1] for the second,
and so forth.  How they order their NICs is part of their stable user
interface.

To leave a NIC unplugged, use -nice none.  Example: -nic none -nic user
leaves the first NIC unplugged, and plugs the second one using a user
network backend.

Say the board contains a SoC that provides four NICs, but the board
wires up only the last one.  Then board code should use nd_table[0] for
that last one.

I don't remember whether network device frontends can work without a
backend, or need a null backend.  If the latter, then board code needs
to supply such null backends.

>> How should we proceed in such cases ?

Model the physical hardware as faithfully as we can.

Follow-up patches welcome!

[PATCH 05/24] aspeed: Don't create unwanted "cortex-a7-arm-cpu" devices

[Markus Armbruster]

The number of CPUs is controlled by property "num-cpus".
aspeed_soc_ast2600_init() creates the maximum supported number.
aspeed_soc_ast2600_realize() realizes only the wanted number.  Works,
although it leaves unrealized devices hanging around in the QOM
composition tree.  Affects machines ast2600-evb and tacoma-bmc.

Make the init functions create only the wanted ones.  Visible in "info
qom-tree"; here's the change for ast2600-evb:

     /machine (ast2600-evb-machine)
       [...]
       /soc (ast2600-a1)
         [...]
         /cpu[0] (cortex-a7-arm-cpu)
           /unnamed-gpio-in[0] (irq)
           /unnamed-gpio-in[1] (irq)
           /unnamed-gpio-in[2] (irq)
           /unnamed-gpio-in[3] (irq)
    -    /cpu[1] (cortex-a7-arm-cpu)
    -      /unnamed-gpio-in[0] (irq)
    -      /unnamed-gpio-in[1] (irq)
    -      /unnamed-gpio-in[2] (irq)
    -      /unnamed-gpio-in[3] (irq)
         /ehci[0] (platform-ehci-usb)

Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Joel Stanley <joel@jms.id.au>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/aspeed_ast2600.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 0a6a77dd54..6ffa587a7f 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -287,6 +287,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
+    for (; i < sc->num_cpus; i++) {
+        object_unparent(OBJECT(&s->cpu[i]));
+    }
 
     /* A7MPCORE */
     object_property_set_int(OBJECT(&s->a7mpcore), s->num_cpus, "num-cpu",
-- 
2.21.1

Re: [PATCH 05/24] aspeed: Don't create unwanted "cortex-a7-arm-cpu" devices

[Cédric Le Goater]

On 5/18/20 7:03 AM, Markus Armbruster wrote:
> The number of CPUs is controlled by property "num-cpus".
> aspeed_soc_ast2600_init() creates the maximum supported number.
> aspeed_soc_ast2600_realize() realizes only the wanted number.  Works,
> although it leaves unrealized devices hanging around in the QOM
> composition tree.  Affects machines ast2600-evb and tacoma-bmc.
> 
> Make the init functions create only the wanted ones.  Visible in "info
> qom-tree"; here's the change for ast2600-evb:
> 
>      /machine (ast2600-evb-machine)
>        [...]
>        /soc (ast2600-a1)
>          [...]
>          /cpu[0] (cortex-a7-arm-cpu)
>            /unnamed-gpio-in[0] (irq)
>            /unnamed-gpio-in[1] (irq)
>            /unnamed-gpio-in[2] (irq)
>            /unnamed-gpio-in[3] (irq)
>     -    /cpu[1] (cortex-a7-arm-cpu)
>     -      /unnamed-gpio-in[0] (irq)
>     -      /unnamed-gpio-in[1] (irq)
>     -      /unnamed-gpio-in[2] (irq)
>     -      /unnamed-gpio-in[3] (irq)
>          /ehci[0] (platform-ehci-usb)
> 
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Joel, Andrew,

Shouldn't we enforce a default/min/max number of CPUs of 2 for the AST2600 ? 
That's the SoC definition. The fact it is configurable in the Aspeed model
was nice to have during bringup but we are now done.  

Thanks,

C.

> ---
>  hw/arm/aspeed_ast2600.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 0a6a77dd54..6ffa587a7f 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -287,6 +287,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> +    for (; i < sc->num_cpus; i++) {
> +        object_unparent(OBJECT(&s->cpu[i]));
> +    }
>  
>      /* A7MPCORE */
>      object_property_set_int(OBJECT(&s->a7mpcore), s->num_cpus, "num-cpu",
> 

Re: [PATCH 05/24] aspeed: Don't create unwanted "cortex-a7-arm-cpu" devices

[Joel Stanley]

On Mon, 18 May 2020 at 12:24, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 5/18/20 7:03 AM, Markus Armbruster wrote:
> > The number of CPUs is controlled by property "num-cpus".
> > aspeed_soc_ast2600_init() creates the maximum supported number.
> > aspeed_soc_ast2600_realize() realizes only the wanted number.  Works,
> > although it leaves unrealized devices hanging around in the QOM
> > composition tree.  Affects machines ast2600-evb and tacoma-bmc.
> >
> > Make the init functions create only the wanted ones.  Visible in "info
> > qom-tree"; here's the change for ast2600-evb:
> >
> >      /machine (ast2600-evb-machine)
> >        [...]
> >        /soc (ast2600-a1)
> >          [...]
> >          /cpu[0] (cortex-a7-arm-cpu)
> >            /unnamed-gpio-in[0] (irq)
> >            /unnamed-gpio-in[1] (irq)
> >            /unnamed-gpio-in[2] (irq)
> >            /unnamed-gpio-in[3] (irq)
> >     -    /cpu[1] (cortex-a7-arm-cpu)
> >     -      /unnamed-gpio-in[0] (irq)
> >     -      /unnamed-gpio-in[1] (irq)
> >     -      /unnamed-gpio-in[2] (irq)
> >     -      /unnamed-gpio-in[3] (irq)
> >          /ehci[0] (platform-ehci-usb)
> >
> > Cc: "Cédric Le Goater" <clg@kaod.org>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Andrew Jeffery <andrew@aj.id.au>
> > Cc: Joel Stanley <joel@jms.id.au>
> > Cc: qemu-arm@nongnu.org
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> Joel, Andrew,
>
> Shouldn't we enforce a default/min/max number of CPUs of 2 for the AST2600 ?
> That's the SoC definition. The fact it is configurable in the Aspeed model
> was nice to have during bringup but we are now done.

Agreed, we want there to always be two CPUs for the 2600.

>
> Thanks,
>
> C.
>
> > ---
> >  hw/arm/aspeed_ast2600.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index 0a6a77dd54..6ffa587a7f 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -287,6 +287,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> >              return;
> >          }
> >      }
> > +    for (; i < sc->num_cpus; i++) {
> > +        object_unparent(OBJECT(&s->cpu[i]));
> > +    }
> >
> >      /* A7MPCORE */
> >      object_property_set_int(OBJECT(&s->a7mpcore), s->num_cpus, "num-cpu",
> >
>

Re: [PATCH 05/24] aspeed: Don't create unwanted "cortex-a7-arm-cpu" devices

[Markus Armbruster]

Joel Stanley <joel@jms.id.au> writes:

> On Mon, 18 May 2020 at 12:24, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 5/18/20 7:03 AM, Markus Armbruster wrote:
>> > The number of CPUs is controlled by property "num-cpus".
>> > aspeed_soc_ast2600_init() creates the maximum supported number.
>> > aspeed_soc_ast2600_realize() realizes only the wanted number.  Works,
>> > although it leaves unrealized devices hanging around in the QOM
>> > composition tree.  Affects machines ast2600-evb and tacoma-bmc.
>> >
>> > Make the init functions create only the wanted ones.  Visible in "info
>> > qom-tree"; here's the change for ast2600-evb:
>> >
>> >      /machine (ast2600-evb-machine)
>> >        [...]
>> >        /soc (ast2600-a1)
>> >          [...]
>> >          /cpu[0] (cortex-a7-arm-cpu)
>> >            /unnamed-gpio-in[0] (irq)
>> >            /unnamed-gpio-in[1] (irq)
>> >            /unnamed-gpio-in[2] (irq)
>> >            /unnamed-gpio-in[3] (irq)
>> >     -    /cpu[1] (cortex-a7-arm-cpu)
>> >     -      /unnamed-gpio-in[0] (irq)
>> >     -      /unnamed-gpio-in[1] (irq)
>> >     -      /unnamed-gpio-in[2] (irq)
>> >     -      /unnamed-gpio-in[3] (irq)
>> >          /ehci[0] (platform-ehci-usb)
>> >
>> > Cc: "Cédric Le Goater" <clg@kaod.org>
>> > Cc: Peter Maydell <peter.maydell@linaro.org>
>> > Cc: Andrew Jeffery <andrew@aj.id.au>
>> > Cc: Joel Stanley <joel@jms.id.au>
>> > Cc: qemu-arm@nongnu.org
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> Joel, Andrew,
>>
>> Shouldn't we enforce a default/min/max number of CPUs of 2 for the AST2600 ?
>> That's the SoC definition. The fact it is configurable in the Aspeed model
>> was nice to have during bringup but we are now done.
>
> Agreed, we want there to always be two CPUs for the 2600.

Follow-up patch welcome!

Re: [PATCH 05/24] aspeed: Don't create unwanted "cortex-a7-arm-cpu" devices

[Cédric Le Goater]

On 5/19/20 7:46 AM, Markus Armbruster wrote:
> Joel Stanley <joel@jms.id.au> writes:
> 
>> On Mon, 18 May 2020 at 12:24, Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> On 5/18/20 7:03 AM, Markus Armbruster wrote:
>>>> The number of CPUs is controlled by property "num-cpus".
>>>> aspeed_soc_ast2600_init() creates the maximum supported number.
>>>> aspeed_soc_ast2600_realize() realizes only the wanted number.  Works,
>>>> although it leaves unrealized devices hanging around in the QOM
>>>> composition tree.  Affects machines ast2600-evb and tacoma-bmc.
>>>>
>>>> Make the init functions create only the wanted ones.  Visible in "info
>>>> qom-tree"; here's the change for ast2600-evb:
>>>>
>>>>      /machine (ast2600-evb-machine)
>>>>        [...]
>>>>        /soc (ast2600-a1)
>>>>          [...]
>>>>          /cpu[0] (cortex-a7-arm-cpu)
>>>>            /unnamed-gpio-in[0] (irq)
>>>>            /unnamed-gpio-in[1] (irq)
>>>>            /unnamed-gpio-in[2] (irq)
>>>>            /unnamed-gpio-in[3] (irq)
>>>>     -    /cpu[1] (cortex-a7-arm-cpu)
>>>>     -      /unnamed-gpio-in[0] (irq)
>>>>     -      /unnamed-gpio-in[1] (irq)
>>>>     -      /unnamed-gpio-in[2] (irq)
>>>>     -      /unnamed-gpio-in[3] (irq)
>>>>          /ehci[0] (platform-ehci-usb)
>>>>
>>>> Cc: "Cédric Le Goater" <clg@kaod.org>
>>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>>> Cc: Andrew Jeffery <andrew@aj.id.au>
>>>> Cc: Joel Stanley <joel@jms.id.au>
>>>> Cc: qemu-arm@nongnu.org
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>
>>> Joel, Andrew,
>>>
>>> Shouldn't we enforce a default/min/max number of CPUs of 2 for the AST2600 ?
>>> That's the SoC definition. The fact it is configurable in the Aspeed model
>>> was nice to have during bringup but we are now done.
>>
>> Agreed, we want there to always be two CPUs for the 2600.
> 
> Follow-up patch welcome!

I just sent a patch on this topic.

C.
 

[PATCH 06/24] armv7m: Bury unwanted "ARM,bitband-memory" devices

[Markus Armbruster]

These devices are optional, and enabled by property "enable-bitband".
armv7m_instance_init() creates them unconditionally, because the
property has not been set then.  armv7m_realize() realizes them only
when the property is true.  Works, although it leaves unrealized
devices hanging around in the QOM composition tree.  Affects machines
microbit, mps2-an505, mps2-an521, musca-a, and musca-b1.

Bury the unwanted devices by making armv7m_realize() unparent them.
Visible in "info qom-tree"; here's the change for microbit:

     /machine (microbit-machine)
       /microbit.twi (microbit.i2c)
         /microbit.twi[0] (qemu:memory-region)
       /nrf51 (nrf51-soc)
         /armv6m (armv7m)
           /armv7m-container[0] (qemu:memory-region)
    -      /bitband[0] (ARM,bitband-memory)
    -        /bitband[0] (qemu:memory-region)
    -      /bitband[1] (ARM,bitband-memory)
    -        /bitband[0] (qemu:memory-region)
           /cpu (cortex-m0-arm-cpu)

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/armv7m.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 7da57f56d3..f930619f53 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -245,8 +245,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->container, 0xe000e000,
                                 sysbus_mmio_get_region(sbd, 0));
 
-    if (s->enable_bitband) {
-        for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
+    for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
+        if (s->enable_bitband) {
             Object *obj = OBJECT(&s->bitband[i]);
             SysBusDevice *sbd = SYS_BUS_DEVICE(&s->bitband[i]);
 
@@ -265,6 +265,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
 
             memory_region_add_subregion(&s->container, bitband_output_addr[i],
                                         sysbus_mmio_get_region(sbd, 0));
+        } else {
+            object_unparent(OBJECT(&s->bitband[i]));
         }
     }
 }
-- 
2.21.1

Re: [PATCH 06/24] armv7m: Bury unwanted "ARM,bitband-memory" devices

[Peter Maydell]

On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
>
> These devices are optional, and enabled by property "enable-bitband".
> armv7m_instance_init() creates them unconditionally, because the
> property has not been set then.  armv7m_realize() realizes them only
> when the property is true.  Works, although it leaves unrealized
> devices hanging around in the QOM composition tree.  Affects machines
> microbit, mps2-an505, mps2-an521, musca-a, and musca-b1.
>
> Bury the unwanted devices by making armv7m_realize() unparent them.
> Visible in "info qom-tree"; here's the change for microbit:
>
>      /machine (microbit-machine)
>        /microbit.twi (microbit.i2c)
>          /microbit.twi[0] (qemu:memory-region)
>        /nrf51 (nrf51-soc)
>          /armv6m (armv7m)
>            /armv7m-container[0] (qemu:memory-region)
>     -      /bitband[0] (ARM,bitband-memory)
>     -        /bitband[0] (qemu:memory-region)
>     -      /bitband[1] (ARM,bitband-memory)
>     -        /bitband[0] (qemu:memory-region)
>            /cpu (cortex-m0-arm-cpu)

What does "bury" mean here? To me it implies "they still
exist but we've stuck them in a hole somewhere and covered
them up", but the qom-tree delta suggests we've actually
really deleted them?

thanks
-- PMM

Re: [PATCH 06/24] armv7m: Bury unwanted "ARM,bitband-memory" devices

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> These devices are optional, and enabled by property "enable-bitband".
>> armv7m_instance_init() creates them unconditionally, because the
>> property has not been set then.  armv7m_realize() realizes them only
>> when the property is true.  Works, although it leaves unrealized
>> devices hanging around in the QOM composition tree.  Affects machines
>> microbit, mps2-an505, mps2-an521, musca-a, and musca-b1.
>>
>> Bury the unwanted devices by making armv7m_realize() unparent them.
>> Visible in "info qom-tree"; here's the change for microbit:
>>
>>      /machine (microbit-machine)
>>        /microbit.twi (microbit.i2c)
>>          /microbit.twi[0] (qemu:memory-region)
>>        /nrf51 (nrf51-soc)
>>          /armv6m (armv7m)
>>            /armv7m-container[0] (qemu:memory-region)
>>     -      /bitband[0] (ARM,bitband-memory)
>>     -        /bitband[0] (qemu:memory-region)
>>     -      /bitband[1] (ARM,bitband-memory)
>>     -        /bitband[0] (qemu:memory-region)
>>            /cpu (cortex-m0-arm-cpu)
>
> What does "bury" mean here? To me it implies "they still
> exist but we've stuck them in a hole somewhere and covered
> them up", but the qom-tree delta suggests we've actually
> really deleted them?

We really delete them now.

"They've been lying dead in the streets; give them a decent burial".

Would you like me to s/Bury/Delete/?

Re: [PATCH 06/24] armv7m: Bury unwanted "ARM,bitband-memory" devices

[Paolo Bonzini]

On 25/05/20 07:50, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
>>> These devices are optional, and enabled by property "enable-bitband".
>>> armv7m_instance_init() creates them unconditionally, because the
>>> property has not been set then.  armv7m_realize() realizes them only
>>> when the property is true.  Works, although it leaves unrealized
>>> devices hanging around in the QOM composition tree.  Affects machines
>>> microbit, mps2-an505, mps2-an521, musca-a, and musca-b1.
>>>
>>> Bury the unwanted devices by making armv7m_realize() unparent them.
>>> Visible in "info qom-tree"; here's the change for microbit:
>>
>> What does "bury" mean here? To me it implies "they still
>> exist but we've stuck them in a hole somewhere and covered
>> them up", but the qom-tree delta suggests we've actually
>> really deleted them?
> 
> We really delete them now.
> 
> "They've been lying dead in the streets; give them a decent burial".
> 
> Would you like me to s/Bury/Delete/?

"Bury unwanted" -> "Dispose of unused"?


Paolo

Re: [PATCH 06/24] armv7m: Bury unwanted "ARM,bitband-memory" devices

[Peter Maydell]

On Mon, 25 May 2020 at 13:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 25/05/20 07:50, Markus Armbruster wrote:
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> >> On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
> >>> These devices are optional, and enabled by property "enable-bitband".
> >>> armv7m_instance_init() creates them unconditionally, because the
> >>> property has not been set then.  armv7m_realize() realizes them only
> >>> when the property is true.  Works, although it leaves unrealized
> >>> devices hanging around in the QOM composition tree.  Affects machines
> >>> microbit, mps2-an505, mps2-an521, musca-a, and musca-b1.
> >>>
> >>> Bury the unwanted devices by making armv7m_realize() unparent them.
> >>> Visible in "info qom-tree"; here's the change for microbit:
> >>
> >> What does "bury" mean here? To me it implies "they still
> >> exist but we've stuck them in a hole somewhere and covered
> >> them up", but the qom-tree delta suggests we've actually
> >> really deleted them?
> >
> > We really delete them now.
> >
> > "They've been lying dead in the streets; give them a decent burial".
> >
> > Would you like me to s/Bury/Delete/?
>
> "Bury unwanted" -> "Dispose of unused"?

Yeah, delete or dispose of would be clearer I think.

thanks
-- PMM

Re: [PATCH 06/24] armv7m: Bury unwanted "ARM,bitband-memory" devices

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 25 May 2020 at 13:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 25/05/20 07:50, Markus Armbruster wrote:
>> > Peter Maydell <peter.maydell@linaro.org> writes:
>> >
>> >> On Mon, 18 May 2020 at 06:04, Markus Armbruster <armbru@redhat.com> wrote:
>> >>> These devices are optional, and enabled by property "enable-bitband".
>> >>> armv7m_instance_init() creates them unconditionally, because the
>> >>> property has not been set then.  armv7m_realize() realizes them only
>> >>> when the property is true.  Works, although it leaves unrealized
>> >>> devices hanging around in the QOM composition tree.  Affects machines
>> >>> microbit, mps2-an505, mps2-an521, musca-a, and musca-b1.
>> >>>
>> >>> Bury the unwanted devices by making armv7m_realize() unparent them.
>> >>> Visible in "info qom-tree"; here's the change for microbit:
>> >>
>> >> What does "bury" mean here? To me it implies "they still
>> >> exist but we've stuck them in a hole somewhere and covered
>> >> them up", but the qom-tree delta suggests we've actually
>> >> really deleted them?
>> >
>> > We really delete them now.
>> >
>> > "They've been lying dead in the streets; give them a decent burial".
>> >
>> > Would you like me to s/Bury/Delete/?
>>
>> "Bury unwanted" -> "Dispose of unused"?
>
> Yeah, delete or dispose of would be clearer I think.

Okay, the subjects are short enough to accomodate a change to 'Delete
unused "..." devices'.

Thanks!

[PATCH 07/24] auxbus: Fix aux-to-i2c-bridge to be a subtype of aux-slave

[Markus Armbruster]

We plug aux-to-i2c-bridge into the aux-bus, even though its
DeviceClass member bus_type is null, not TYPE_AUX_BUS.  Fix that by
deriving it from TYPE_AUX_SLAVE instead of TYPE_DEVICE.

Cc: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/auxbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index f8e7b97971..5e4794f0ac 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -244,7 +244,7 @@ static inline I2CBus *aux_bridge_get_i2c_bus(AUXTOI2CState *bridge)
 
 static const TypeInfo aux_to_i2c_type_info = {
     .name = TYPE_AUXTOI2C,
-    .parent = TYPE_DEVICE,
+    .parent = TYPE_AUX_SLAVE,
     .class_init = aux_bridge_class_init,
     .instance_size = sizeof(AUXTOI2CState),
     .instance_init = aux_bridge_init
-- 
2.21.1

Re: [PATCH 07/24] auxbus: Fix aux-to-i2c-bridge to be a subtype of aux-slave

[Philippe Mathieu-Daudé]

On 5/18/20 7:03 AM, Markus Armbruster wrote:
> We plug aux-to-i2c-bridge into the aux-bus, even though its
> DeviceClass member bus_type is null, not TYPE_AUX_BUS.  Fix that by
> deriving it from TYPE_AUX_SLAVE instead of TYPE_DEVICE.
> 
> Cc: KONRAD Frederic <fred.konrad@greensocs.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/misc/auxbus.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
> index f8e7b97971..5e4794f0ac 100644
> --- a/hw/misc/auxbus.c
> +++ b/hw/misc/auxbus.c
> @@ -244,7 +244,7 @@ static inline I2CBus *aux_bridge_get_i2c_bus(AUXTOI2CState *bridge)
>   
>   static const TypeInfo aux_to_i2c_type_info = {
>       .name = TYPE_AUXTOI2C,
> -    .parent = TYPE_DEVICE,
> +    .parent = TYPE_AUX_SLAVE,
>       .class_init = aux_bridge_class_init,
>       .instance_size = sizeof(AUXTOI2CState),
>       .instance_init = aux_bridge_init
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 08/24] mac_via: Fix to realize "mos6522-q800-via*" devices

[Markus Armbruster]

mac_via_realize() creates a "mos6522-q800-via1" and a
"mos6522-q800-via2" device, but neglects to realize them.  Affects
machine q800.

I wonder how this ever worked.  If the "device becomes real only on
realize" thing actually works, then we've always been missing these
two devices, yet nobody noticed.

Fix by realizing them right away.

Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
Cc: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/mac_via.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index e05623d730..ee32f72d75 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -890,6 +890,9 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
     object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
                               SYSBUS_DEVICE_GPIO_IRQ "[0]");
 
+    qdev_init_nofail(DEVICE(&m->mos6522_via1));
+    qdev_init_nofail(DEVICE(&m->mos6522_via2));
+
     /* Pass through mos6522 input IRQs */
     qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq");
     qdev_pass_gpios(DEVICE(&m->mos6522_via2), dev, "via2-irq");
-- 
2.21.1

Re: [PATCH 08/24] mac_via: Fix to realize "mos6522-q800-via*" devices

[Mark Cave-Ayland]

On 18/05/2020 06:03, Markus Armbruster wrote:

> mac_via_realize() creates a "mos6522-q800-via1" and a
> "mos6522-q800-via2" device, but neglects to realize them.  Affects
> machine q800.
> 
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing these
> two devices, yet nobody noticed.
> 
> Fix by realizing them right away.
> 
> Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
> Cc: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/mac_via.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index e05623d730..ee32f72d75 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -890,6 +890,9 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>      object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
>                                SYSBUS_DEVICE_GPIO_IRQ "[0]");
>  
> +    qdev_init_nofail(DEVICE(&m->mos6522_via1));
> +    qdev_init_nofail(DEVICE(&m->mos6522_via2));
> +
>      /* Pass through mos6522 input IRQs */
>      qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq");
>      qdev_pass_gpios(DEVICE(&m->mos6522_via2), dev, "via2-irq");

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Markus Armbruster]

cuda_init() creates a "mos6522-cuda" device, but it's never realized.
Affects machines mac99 with via=cuda (default) and g3beige.

pmu_init() creates a "mos6522-pmu" device, but it's never realized.
Affects machine mac99 with via=pmu and via=pmu-adb,

I wonder how this ever worked.  If the "device becomes real only on
realize" thing actually works, then we've always been missing these
devices, yet nobody noticed.

Fix by realizing them in cuda_realize() and pmu_realize(),
respectively.

Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
Cc: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/macio/cuda.c | 8 +++-----
 hw/misc/macio/pmu.c  | 8 +++-----
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index e0cc0aac5d..6d4d135f71 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -523,15 +523,13 @@ static void cuda_realize(DeviceState *dev, Error **errp)
 {
     CUDAState *s = CUDA(dev);
     SysBusDevice *sbd;
-    MOS6522State *ms;
-    DeviceState *d;
     struct tm tm;
 
+    qdev_init_nofail(DEVICE(&s->mos6522_cuda));
+
     /* Pass IRQ from 6522 */
-    d = DEVICE(&s->mos6522_cuda);
-    ms = MOS6522(d);
     sbd = SYS_BUS_DEVICE(s);
-    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
+    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->mos6522_cuda));
 
     qemu_get_timedate(&tm, 0);
     s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index 9a9cd427e1..e29ca5e6cc 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -740,15 +740,13 @@ static void pmu_realize(DeviceState *dev, Error **errp)
 {
     PMUState *s = VIA_PMU(dev);
     SysBusDevice *sbd;
-    MOS6522State *ms;
-    DeviceState *d;
     struct tm tm;
 
+    qdev_init_nofail(DEVICE(&s->mos6522_pmu));
+
     /* Pass IRQ from 6522 */
-    d = DEVICE(&s->mos6522_pmu);
-    ms = MOS6522(d);
     sbd = SYS_BUS_DEVICE(s);
-    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
+    sysbus_pass_irq(sbd, SYS_BUS_DEVICE(&s->mos6522_pmu));
 
     qemu_get_timedate(&tm, 0);
     s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
-- 
2.21.1

Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Peter Maydell]

On Mon, 18 May 2020 at 06:12, Markus Armbruster <armbru@redhat.com> wrote:
>
> cuda_init() creates a "mos6522-cuda" device, but it's never realized.
> Affects machines mac99 with via=cuda (default) and g3beige.
>
> pmu_init() creates a "mos6522-pmu" device, but it's never realized.
> Affects machine mac99 with via=pmu and via=pmu-adb,
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing these
> devices, yet nobody noticed.

Same remark as elsewhere: the devices aren't missing, we just
got lucky that using them in a half-initialized state happens
to work for these devices. Could you update the commit messages
when you reroll this series, please?

> Fix by realizing them in cuda_realize() and pmu_realize(),
> respectively.
>
> Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
> Cc: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/macio/cuda.c | 8 +++-----
>  hw/misc/macio/pmu.c  | 8 +++-----
>  2 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index e0cc0aac5d..6d4d135f71 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -523,15 +523,13 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>  {
>      CUDAState *s = CUDA(dev);
>      SysBusDevice *sbd;
> -    MOS6522State *ms;
> -    DeviceState *d;
>      struct tm tm;
>
> +    qdev_init_nofail(DEVICE(&s->mos6522_cuda));

Since we init the device with sysbus_init_child_obj() and
we're in a position here to propagate any realize error
back up to our caller, it would be nicer to do this via
setting the realize property rather than qdev_init_nofail().

That's what this patch from March does:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg04260.html
(we seem to have let that series drop accidentally,
probably because it was halfway through release and
because it touches several architectures at once).

thanks
-- PMM

Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 18 May 2020 at 06:12, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> cuda_init() creates a "mos6522-cuda" device, but it's never realized.
>> Affects machines mac99 with via=cuda (default) and g3beige.
>>
>> pmu_init() creates a "mos6522-pmu" device, but it's never realized.
>> Affects machine mac99 with via=pmu and via=pmu-adb,
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing these
>> devices, yet nobody noticed.
>
> Same remark as elsewhere: the devices aren't missing, we just
> got lucky that using them in a half-initialized state happens
> to work for these devices. Could you update the commit messages
> when you reroll this series, please?

Of course.  What about something like this:

    In theory, a device becomes real only on realize.  In practice, the
    transition from unreal to real is a fuzzy one.  The work to make a
    device real can be spread between realize methods (fine),
    instance_init methods (wrong), and board code wiring up the device
    (fine as long as it effectively happens on realize).  Depending on
    what exactly is done where, a device can work even when we neglect
    to realize it.  Nevertheless, it's a clear misuse of the interface.
    Even when it works today (more or less by chance), it can break
    tomorrow.

>> Fix by realizing them in cuda_realize() and pmu_realize(),
>> respectively.
>>
>> Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
>> Cc: Laurent Vivier <laurent@vivier.eu>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/misc/macio/cuda.c | 8 +++-----
>>  hw/misc/macio/pmu.c  | 8 +++-----
>>  2 files changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index e0cc0aac5d..6d4d135f71 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -523,15 +523,13 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>>  {
>>      CUDAState *s = CUDA(dev);
>>      SysBusDevice *sbd;
>> -    MOS6522State *ms;
>> -    DeviceState *d;
>>      struct tm tm;
>>
>> +    qdev_init_nofail(DEVICE(&s->mos6522_cuda));
>
> Since we init the device with sysbus_init_child_obj() and
> we're in a position here to propagate any realize error
> back up to our caller, it would be nicer to do this via
> setting the realize property rather than qdev_init_nofail().

The error handling will be unreachable.

The proper way to say "error not possible" is of course &error_abort,
not qdev_init_nofail()'s &error_fatal.

Many realize methods misuse the Error interface this way.  NULL instead
of &error_abort is also common.  Cleaning this up is yet another big
task.  But that's no excuse for making it bigger now.

Laurent, would you prefer unreachable error handling or &error_abort?

> That's what this patch from March does:
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg04260.html
> (we seem to have let that series drop accidentally,
> probably because it was halfway through release and
> because it touches several architectures at once).

Pity.

Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

[...]
> Since we init the device with sysbus_init_child_obj() and
> we're in a position here to propagate any realize error
> back up to our caller, it would be nicer to do this via
> setting the realize property rather than qdev_init_nofail().

I checked all my patches for new uses of qdev_init_nofail():

* PATCH 02: in realize(), but right next to existing uses.  I'm not
  making it worse.

* PATCH 03: in helper functions used by board init functions.  Okay.

* PATCH 08: in a realize method.  Can't actually fail, so let's use
  &error_abort.

* PATCH 09 (this one): likewise.

* PATCH 18: in a realize method, and in a realize helper that can't
  fail.  One of the callers uses qdev_init_nofail() already.  Let's Use
  &error_abort.

[...]

Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Peter Maydell]

On Wed, 27 May 2020 at 15:12, Markus Armbruster <armbru@redhat.com> wrote:
> * PATCH 08: in a realize method.  Can't actually fail, so let's use
>   &error_abort.
>
> * PATCH 09 (this one): likewise.

I disagree with these. We're in a realize function, the API
says "on errors, report them via the Error* you got passed",
so we should do that, not blow up. &error_abort only makes
sense if (a) we have no better way to report errors than
to abort (which isn't the case here) or (b) if we can guarantee
that in fact the thing we're doing won't ever fail
(which we can't here without knowing more about the internal
implementation details of the MOS6522 device than we
really ought to).

thanks
-- PMM

Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Paolo Bonzini]

On 27/05/20 17:05, Peter Maydell wrote:
> I disagree with these. We're in a realize function, the API
> says "on errors, report them via the Error* you got passed",
> so we should do that, not blow up. &error_abort only makes
> sense if (a) we have no better way to report errors than
> to abort (which isn't the case here) or (b) if we can guarantee
> that in fact the thing we're doing won't ever fail
> (which we can't here without knowing more about the internal
> implementation details of the MOS6522 device than we
> really ought to).

Note however that before replacing &error_abort with error propagation
you need to make sure that you are "un-realizing" yourself properly.  So
it may be better to have inferior (but clearly visible) error
propagation behavior, than untested (and perhaps untestable) buggy code
that looks great on the surface.

Paolo

Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 27/05/20 17:05, Peter Maydell wrote:
>> I disagree with these. We're in a realize function, the API
>> says "on errors, report them via the Error* you got passed",
>> so we should do that, not blow up. &error_abort only makes
>> sense if (a) we have no better way to report errors than
>> to abort (which isn't the case here) or (b) if we can guarantee
>> that in fact the thing we're doing won't ever fail
>> (which we can't here without knowing more about the internal
>> implementation details of the MOS6522 device than we
>> really ought to).
>
> Note however that before replacing &error_abort with error propagation
> you need to make sure that you are "un-realizing" yourself properly.  So
> it may be better to have inferior (but clearly visible) error
> propagation behavior, than untested (and perhaps untestable) buggy code
> that looks great on the surface.

This is exactly why I have to stop at &error_abort in this series.  It's
24 patches of fixes to enable 50+ patches of refactoring, with more in
the pipeline.  If I stray even deeper into the weeds, my pipeline is
going to explode %-}

Re: [PATCH 09/24] macio: Fix to realize "mos6522-cuda" and "mos6522-pmu" devices

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 27 May 2020 at 15:12, Markus Armbruster <armbru@redhat.com> wrote:
>> * PATCH 08: in a realize method.  Can't actually fail, so let's use
>>   &error_abort.
>>
>> * PATCH 09 (this one): likewise.
>
> I disagree with these. We're in a realize function, the API
> says "on errors, report them via the Error* you got passed",
> so we should do that, not blow up. &error_abort only makes
> sense if (a) we have no better way to report errors than
> to abort (which isn't the case here) or (b) if we can guarantee
> that in fact the thing we're doing won't ever fail

I detest impossible (and therefore untestable) error paths.

> (which we can't here without knowing more about the internal
> implementation details of the MOS6522 device than we
> really ought to).

At least the child devices are all defined in the same file.

My second line of defense: my patches are strict improvments.  If you
want further improvements, I'd prefer you propose them as patches on top
of mine.

[PATCH 10/24] macio: Bury unwanted "macio-gpio" devices

[Markus Armbruster]

These devices go with the "via-pmu" device, which is controlled by
property "has-pmu".  macio_newworld_init() creates it unconditionally,
because the property has not been set then.  macio_newworld_realize()
realizes it only when the property is true.  Works, although it can
leave an unrealized device hanging around in the QOM composition tree.
Affects machine mac99 with via=cuda (default).

Bury the unwanted device by making macio_newworld_realize() unparent
it.  Visible in "info qom-tree":

     /machine (mac99-machine)
       [...]
       /unattached (container)
         /device[9] (macio-newworld)
           [...]
           /escc-legacy-port[8] (qemu:memory-region)
           /escc-legacy-port[9] (qemu:memory-region)
           /escc-legacy[0] (qemu:memory-region)
    -      /gpio (macio-gpio)
    -        /gpio[0] (qemu:memory-region)
           /ide[0] (macio-ide)
             /ide.0 (IDE)
             /pmac-ide[0] (qemu:memory-region)

Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/macio/macio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 3779865ab2..b3dddf8be7 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -368,6 +368,8 @@ static void macio_newworld_realize(PCIDevice *d, Error **errp)
         memory_region_add_subregion(&s->bar, 0x16000,
                                     sysbus_mmio_get_region(sysbus_dev, 0));
     } else {
+        object_unparent(OBJECT(&ns->gpio));
+
         /* CUDA */
         object_initialize_child(OBJECT(s), "cuda", &s->cuda, sizeof(s->cuda),
                                 TYPE_CUDA, &error_abort, NULL);
-- 
2.21.1

Re: [PATCH 10/24] macio: Bury unwanted "macio-gpio" devices

[Mark Cave-Ayland]

On 18/05/2020 06:03, Markus Armbruster wrote:

> These devices go with the "via-pmu" device, which is controlled by
> property "has-pmu".  macio_newworld_init() creates it unconditionally,
> because the property has not been set then.  macio_newworld_realize()
> realizes it only when the property is true.  Works, although it can
> leave an unrealized device hanging around in the QOM composition tree.
> Affects machine mac99 with via=cuda (default).
> 
> Bury the unwanted device by making macio_newworld_realize() unparent
> it.  Visible in "info qom-tree":
> 
>      /machine (mac99-machine)
>        [...]
>        /unattached (container)
>          /device[9] (macio-newworld)
>            [...]
>            /escc-legacy-port[8] (qemu:memory-region)
>            /escc-legacy-port[9] (qemu:memory-region)
>            /escc-legacy[0] (qemu:memory-region)
>     -      /gpio (macio-gpio)
>     -        /gpio[0] (qemu:memory-region)
>            /ide[0] (macio-ide)
>              /ide.0 (IDE)
>              /pmac-ide[0] (qemu:memory-region)
> 
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/macio/macio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 3779865ab2..b3dddf8be7 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -368,6 +368,8 @@ static void macio_newworld_realize(PCIDevice *d, Error **errp)
>          memory_region_add_subregion(&s->bar, 0x16000,
>                                      sysbus_mmio_get_region(sysbus_dev, 0));
>      } else {
> +        object_unparent(OBJECT(&ns->gpio));
> +
>          /* CUDA */
>          object_initialize_child(OBJECT(s), "cuda", &s->cuda, sizeof(s->cuda),
>                                  TYPE_CUDA, &error_abort, NULL);

This one is a little more interesting because it comes back to the previous
discussions around if you have a device that contains other devices, should you init
all the children in your container device init, and the realize all your children in
your container device realize?

If so I guess this patch isn't technically wrong, but it is somewhat misleading given
that the existing init/realize pattern here is incorrect. Perhaps it should go ahead
and make everything work the "right way"?


ATB,

Mark.

Re: [PATCH 10/24] macio: Bury unwanted "macio-gpio" devices

[Markus Armbruster]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> On 18/05/2020 06:03, Markus Armbruster wrote:
>
>> These devices go with the "via-pmu" device, which is controlled by
>> property "has-pmu".  macio_newworld_init() creates it unconditionally,
>> because the property has not been set then.  macio_newworld_realize()
>> realizes it only when the property is true.  Works, although it can
>> leave an unrealized device hanging around in the QOM composition tree.
>> Affects machine mac99 with via=cuda (default).
>> 
>> Bury the unwanted device by making macio_newworld_realize() unparent
>> it.  Visible in "info qom-tree":
>> 
>>      /machine (mac99-machine)
>>        [...]
>>        /unattached (container)
>>          /device[9] (macio-newworld)
>>            [...]
>>            /escc-legacy-port[8] (qemu:memory-region)
>>            /escc-legacy-port[9] (qemu:memory-region)
>>            /escc-legacy[0] (qemu:memory-region)
>>     -      /gpio (macio-gpio)
>>     -        /gpio[0] (qemu:memory-region)
>>            /ide[0] (macio-ide)
>>              /ide.0 (IDE)
>>              /pmac-ide[0] (qemu:memory-region)
>> 
>> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Cc: qemu-ppc@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/misc/macio/macio.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index 3779865ab2..b3dddf8be7 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -368,6 +368,8 @@ static void macio_newworld_realize(PCIDevice *d, Error **errp)
>>          memory_region_add_subregion(&s->bar, 0x16000,
>>                                      sysbus_mmio_get_region(sysbus_dev, 0));
>>      } else {
>> +        object_unparent(OBJECT(&ns->gpio));
>> +
>>          /* CUDA */
>>          object_initialize_child(OBJECT(s), "cuda", &s->cuda, sizeof(s->cuda),
>>                                  TYPE_CUDA, &error_abort, NULL);
>
> This one is a little more interesting because it comes back to the previous
> discussions around if you have a device that contains other devices, should you init
> all the children in your container device init, and the realize all your children in
> your container device realize?

You have to initialize them in the container's instance_init method to
make their properties accessible.

You have to realize them in the container's realize method if
realization can fail, or if it has visible side effects.

Many, many places keep initialization and realization together.
Historical reasons, ignorance, laziness, all excusable.

Doing both in realize is safe (I think), but you'll have to refactor
when you need to expose the properties for configuration.  Cleaning that
up proactively feels unnecessary.

Doing both in instance_init necessitates a fragile, non-local
correctness argument around "can't fail" and "doesn't do anything
untoward".  Best avoided, I think.

> If so I guess this patch isn't technically wrong, but it is somewhat misleading given
> that the existing init/realize pattern here is incorrect. Perhaps it should go ahead
> and make everything work the "right way"?

The code being patched here works the nice way: instance_init method
macio_newworld_init() initializes ns->gpio, and realize method
macio_realize_ide() realizes it.  Let's keep it that way.

[PATCH 11/24] pnv/phb4: Bury unwanted "pnv-phb4-pec-stack" devices

[Markus Armbruster]

The number of stacks is controlled by property "num-stacks".
pnv_pec_instance_init() creates the maximum supported number, because
the property has not been set then.  pnv_pec_realize() realizes only
the wanted number.  Works, although it can leave unrealized devices
hanging around in the QOM composition tree.  Affects machine powernv9.

Bury the unwanted devices by making pnv_pec_realize() unparent them.
Visible in "info qom-tree":

     /machine (powernv9-machine)
       /chip[0] (power9_v2.0-pnv-chip)
         [...]
         /pec[0] (pnv-phb4-pec)
           /stack[0] (pnv-phb4-pec-stack)
             [...]
    -      /stack[1] (pnv-phb4-pec-stack)
    -        /phb (pnv-phb4)
    -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
    -          /root (pnv-phb4-root-port)
    -          /source (xive-source)
    -      /stack[2] (pnv-phb4-pec-stack)
    -        /phb (pnv-phb4)
    -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
    -          /root (pnv-phb4-root-port)
    -          /source (xive-source)
           /xscom-pec-0.0-nest[0] (qemu:memory-region)
           /xscom-pec-0.0-pci[0] (qemu:memory-region)
         /pec[1] (pnv-phb4-pec)
           /stack[0] (pnv-phb4-pec-stack)
             [...]
           /stack[1] (pnv-phb4-pec-stack)
             [...]
    -      /stack[2] (pnv-phb4-pec-stack)
    -        /phb (pnv-phb4)
    -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
    -          /root (pnv-phb4-root-port)
    -          /source (xive-source)
           /xscom-pec-0.1-nest[0] (qemu:memory-region)
           /xscom-pec-0.1-pci[0] (qemu:memory-region)
         /pec[2] (pnv-phb4-pec)
           /stack[0] (pnv-phb4-pec-stack)
             [...]
           /stack[1] (pnv-phb4-pec-stack)
             [...]
           /stack[2] (pnv-phb4-pec-stack)
             [...]

Cc: Cédric Le Goater <clg@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pci-host/pnv_phb4_pec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 911d147ffd..565345a018 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -397,6 +397,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
+    for (; i < PHB4_PEC_MAX_STACKS; i++) {
+        object_unparent(OBJECT(&pec->stacks[i]));
+    }
 
     /* Initialize the XSCOM regions for the PEC registers */
     snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest", pec->chip_id,
-- 
2.21.1

Re: [PATCH 11/24] pnv/phb4: Bury unwanted "pnv-phb4-pec-stack" devices

[Greg Kurz]

On Mon, 18 May 2020 07:03:55 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> The number of stacks is controlled by property "num-stacks".
> pnv_pec_instance_init() creates the maximum supported number, because
> the property has not been set then.  pnv_pec_realize() realizes only
> the wanted number.  Works, although it can leave unrealized devices
> hanging around in the QOM composition tree.  Affects machine powernv9.
> 
> Bury the unwanted devices by making pnv_pec_realize() unparent them.
> Visible in "info qom-tree":
> 
>      /machine (powernv9-machine)
>        /chip[0] (power9_v2.0-pnv-chip)
>          [...]
>          /pec[0] (pnv-phb4-pec)
>            /stack[0] (pnv-phb4-pec-stack)
>              [...]
>     -      /stack[1] (pnv-phb4-pec-stack)
>     -        /phb (pnv-phb4)
>     -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
>     -          /root (pnv-phb4-root-port)
>     -          /source (xive-source)
>     -      /stack[2] (pnv-phb4-pec-stack)
>     -        /phb (pnv-phb4)
>     -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
>     -          /root (pnv-phb4-root-port)
>     -          /source (xive-source)
>            /xscom-pec-0.0-nest[0] (qemu:memory-region)
>            /xscom-pec-0.0-pci[0] (qemu:memory-region)
>          /pec[1] (pnv-phb4-pec)
>            /stack[0] (pnv-phb4-pec-stack)
>              [...]
>            /stack[1] (pnv-phb4-pec-stack)
>              [...]
>     -      /stack[2] (pnv-phb4-pec-stack)
>     -        /phb (pnv-phb4)
>     -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
>     -          /root (pnv-phb4-root-port)
>     -          /source (xive-source)
>            /xscom-pec-0.1-nest[0] (qemu:memory-region)
>            /xscom-pec-0.1-pci[0] (qemu:memory-region)
>          /pec[2] (pnv-phb4-pec)
>            /stack[0] (pnv-phb4-pec-stack)
>              [...]
>            /stack[1] (pnv-phb4-pec-stack)
>              [...]
>            /stack[2] (pnv-phb4-pec-stack)
>              [...]
> 
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Makes sense :)

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/pci-host/pnv_phb4_pec.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 911d147ffd..565345a018 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -397,6 +397,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> +    for (; i < PHB4_PEC_MAX_STACKS; i++) {
> +        object_unparent(OBJECT(&pec->stacks[i]));
> +    }
>  
>      /* Initialize the XSCOM regions for the PEC registers */
>      snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest", pec->chip_id,

Re: [PATCH 11/24] pnv/phb4: Bury unwanted "pnv-phb4-pec-stack" devices

[Cédric Le Goater]

On 5/18/20 7:03 AM, Markus Armbruster wrote:
> The number of stacks is controlled by property "num-stacks".
> pnv_pec_instance_init() creates the maximum supported number, because
> the property has not been set then.  pnv_pec_realize() realizes only
> the wanted number.  Works, although it can leave unrealized devices
> hanging around in the QOM composition tree.  Affects machine powernv9.



> Bury the unwanted devices by making pnv_pec_realize() unparent them.
> Visible in "info qom-tree":
> 
>      /machine (powernv9-machine)
>        /chip[0] (power9_v2.0-pnv-chip)
>          [...]
>          /pec[0] (pnv-phb4-pec)
>            /stack[0] (pnv-phb4-pec-stack)
>              [...]
>     -      /stack[1] (pnv-phb4-pec-stack)
>     -        /phb (pnv-phb4)
>     -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
>     -          /root (pnv-phb4-root-port)
>     -          /source (xive-source)
>     -      /stack[2] (pnv-phb4-pec-stack)
>     -        /phb (pnv-phb4)
>     -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
>     -          /root (pnv-phb4-root-port)
>     -          /source (xive-source)
>            /xscom-pec-0.0-nest[0] (qemu:memory-region)
>            /xscom-pec-0.0-pci[0] (qemu:memory-region)
>          /pec[1] (pnv-phb4-pec)
>            /stack[0] (pnv-phb4-pec-stack)
>              [...]
>            /stack[1] (pnv-phb4-pec-stack)
>              [...]
>     -      /stack[2] (pnv-phb4-pec-stack)
>     -        /phb (pnv-phb4)
>     -          /pcie-mmcfg-mmio[0] (qemu:memory-region)
>     -          /root (pnv-phb4-root-port)
>     -          /source (xive-source)
>            /xscom-pec-0.1-nest[0] (qemu:memory-region)
>            /xscom-pec-0.1-pci[0] (qemu:memory-region)
>          /pec[2] (pnv-phb4-pec)
>            /stack[0] (pnv-phb4-pec-stack)
>              [...]
>            /stack[1] (pnv-phb4-pec-stack)
>              [...]
>            /stack[2] (pnv-phb4-pec-stack)
>              [...]
> 
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

> ---
>  hw/pci-host/pnv_phb4_pec.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 911d147ffd..565345a018 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -397,6 +397,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> +    for (; i < PHB4_PEC_MAX_STACKS; i++) {
> +        object_unparent(OBJECT(&pec->stacks[i]));
> +    }
>  
>      /* Initialize the XSCOM regions for the PEC registers */
>      snprintf(name, sizeof(name), "xscom-pec-%d.%d-nest", pec->chip_id,
> 

Re: [PATCH 11/24] pnv/phb4: Bury unwanted "pnv-phb4-pec-stack" devices

[Cédric Le Goater]

On 5/18/20 7:03 AM, Markus Armbruster wrote:
> The number of stacks is controlled by property "num-stacks".
> pnv_pec_instance_init() creates the maximum supported number, because
> the property has not been set then.  pnv_pec_realize() realizes only
> the wanted number.  Works, although it can leave unrealized devices
> hanging around in the QOM composition tree.  Affects machine powernv9.

I have used this pattern in many models. Is there a better one ?

Thanks,

C.

Re: [PATCH 11/24] pnv/phb4: Bury unwanted "pnv-phb4-pec-stack" devices

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 5/18/20 7:03 AM, Markus Armbruster wrote:
>> The number of stacks is controlled by property "num-stacks".
>> pnv_pec_instance_init() creates the maximum supported number, because
>> the property has not been set then.  pnv_pec_realize() realizes only
>> the wanted number.  Works, although it can leave unrealized devices
>> hanging around in the QOM composition tree.  Affects machine powernv9.
>
> I have used this pattern in many models. Is there a better one ?

The pattern is just fine, we just need to unparent any devices that turn
out to be unwanted.

Of course, when we already know what's wanted at instance_init time,
there's no reason for creating more.

[PATCH 12/24] MAINTAINERS: Make section PowerNV cover pci-host/pnv* as well

[Markus Armbruster]

Cc: Cédric Le Goater <clg@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 47ef3139e6..074dc7f023 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1207,7 +1207,9 @@ S: Maintained
 F: hw/ppc/pnv*
 F: hw/intc/pnv*
 F: hw/intc/xics_pnv.c
+F: hw/pci-host/pnv*
 F: include/hw/ppc/pnv*
+F: include/hw/pci-host/pnv*
 F: pc-bios/skiboot.lid
 F: tests/qtest/pnv*
 
-- 
2.21.1

Re: [PATCH 12/24] MAINTAINERS: Make section PowerNV cover pci-host/pnv* as well

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Mon, May 18, 2020 at 07:03:56AM +0200, Markus Armbruster wrote:
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 47ef3139e6..074dc7f023 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1207,7 +1207,9 @@ S: Maintained
>  F: hw/ppc/pnv*
>  F: hw/intc/pnv*
>  F: hw/intc/xics_pnv.c
> +F: hw/pci-host/pnv*
>  F: include/hw/ppc/pnv*
> +F: include/hw/pci-host/pnv*
>  F: pc-bios/skiboot.lid
>  F: tests/qtest/pnv*
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 13/24] ppc4xx: Drop redundant device realization

[Markus Armbruster]

object_property_set_bool(OBJECT(dev), true, "realized", ...) right
after qdev_init_nofail(dev) does nothing, because qdev_init_nofail()
already realizes.  Drop.

Cc: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/ppc440_uc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index b30e093cbb..dc318c7aa7 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -1370,12 +1370,10 @@ void ppc460ex_pcie_init(CPUPPCState *env)
     dev = qdev_create(NULL, TYPE_PPC460EX_PCIE_HOST);
     qdev_prop_set_int32(dev, "dcrn-base", DCRN_PCIE0_BASE);
     qdev_init_nofail(dev);
-    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
     ppc460ex_pcie_register_dcrs(PPC460EX_PCIE_HOST(dev), env);
 
     dev = qdev_create(NULL, TYPE_PPC460EX_PCIE_HOST);
     qdev_prop_set_int32(dev, "dcrn-base", DCRN_PCIE1_BASE);
     qdev_init_nofail(dev);
-    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
     ppc460ex_pcie_register_dcrs(PPC460EX_PCIE_HOST(dev), env);
 }
-- 
2.21.1

Re: [PATCH 13/24] ppc4xx: Drop redundant device realization

[Philippe Mathieu-Daudé]

On 5/18/20 7:03 AM, Markus Armbruster wrote:
> object_property_set_bool(OBJECT(dev), true, "realized", ...) right
> after qdev_init_nofail(dev) does nothing, because qdev_init_nofail()
> already realizes.  Drop.
> 
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/ppc/ppc440_uc.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index b30e093cbb..dc318c7aa7 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -1370,12 +1370,10 @@ void ppc460ex_pcie_init(CPUPPCState *env)
>       dev = qdev_create(NULL, TYPE_PPC460EX_PCIE_HOST);
>       qdev_prop_set_int32(dev, "dcrn-base", DCRN_PCIE0_BASE);
>       qdev_init_nofail(dev);
> -    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
>       ppc460ex_pcie_register_dcrs(PPC460EX_PCIE_HOST(dev), env);
>   
>       dev = qdev_create(NULL, TYPE_PPC460EX_PCIE_HOST);
>       qdev_prop_set_int32(dev, "dcrn-base", DCRN_PCIE1_BASE);
>       qdev_init_nofail(dev);
> -    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
>       ppc460ex_pcie_register_dcrs(PPC460EX_PCIE_HOST(dev), env);
>   }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 13/24] ppc4xx: Drop redundant device realization

[BALATON Zoltan]

On Mon, 18 May 2020, Markus Armbruster wrote:
> object_property_set_bool(OBJECT(dev), true, "realized", ...) right
> after qdev_init_nofail(dev) does nothing, because qdev_init_nofail()
> already realizes.  Drop.
>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>

Shouldn't this Cc line come after the --- so it's not included in the 
final commit? Thanks.

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/ppc/ppc440_uc.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index b30e093cbb..dc318c7aa7 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -1370,12 +1370,10 @@ void ppc460ex_pcie_init(CPUPPCState *env)
>     dev = qdev_create(NULL, TYPE_PPC460EX_PCIE_HOST);
>     qdev_prop_set_int32(dev, "dcrn-base", DCRN_PCIE0_BASE);
>     qdev_init_nofail(dev);
> -    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
>     ppc460ex_pcie_register_dcrs(PPC460EX_PCIE_HOST(dev), env);
>
>     dev = qdev_create(NULL, TYPE_PPC460EX_PCIE_HOST);
>     qdev_prop_set_int32(dev, "dcrn-base", DCRN_PCIE1_BASE);
>     qdev_init_nofail(dev);
> -    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
>     ppc460ex_pcie_register_dcrs(PPC460EX_PCIE_HOST(dev), env);
> }
>

Re: [PATCH 13/24] ppc4xx: Drop redundant device realization

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Mon, 18 May 2020, Markus Armbruster wrote:
>> object_property_set_bool(OBJECT(dev), true, "realized", ...) right
>> after qdev_init_nofail(dev) does nothing, because qdev_init_nofail()
>> already realizes.  Drop.
>>
>> Cc: BALATON Zoltan <balaton@eik.bme.hu>
>
> Shouldn't this Cc line come after the --- so it's not included in the
> final commit? Thanks.

We routinely include it in git history.

> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Thanks!

Re: [PATCH 13/24] ppc4xx: Drop redundant device realization

[Thomas Huth]

On 18/05/2020 07.03, Markus Armbruster wrote:
> object_property_set_bool(OBJECT(dev), true, "realized", ...) right
> after qdev_init_nofail(dev) does nothing, because qdev_init_nofail()
> already realizes.  Drop.
> 
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/ppc440_uc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index b30e093cbb..dc318c7aa7 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -1370,12 +1370,10 @@ void ppc460ex_pcie_init(CPUPPCState *env)
>      dev = qdev_create(NULL, TYPE_PPC460EX_PCIE_HOST);
>      qdev_prop_set_int32(dev, "dcrn-base", DCRN_PCIE0_BASE);
>      qdev_init_nofail(dev);
> -    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
>      ppc460ex_pcie_register_dcrs(PPC460EX_PCIE_HOST(dev), env);
>  
>      dev = qdev_create(NULL, TYPE_PPC460EX_PCIE_HOST);
>      qdev_prop_set_int32(dev, "dcrn-base", DCRN_PCIE1_BASE);
>      qdev_init_nofail(dev);
> -    object_property_set_bool(OBJECT(dev), true, "realized", NULL);
>      ppc460ex_pcie_register_dcrs(PPC460EX_PCIE_HOST(dev), env);
>  }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 14/24] macio: Put "macio-nvram" device on the macio bus

[Markus Armbruster]

macio_oldworld_init() creates a "macio-nvram", sysbus device, but
neglects to but it on a bus.

Put it on the macio bus.  Affects machine g3beige.  Visible in "info
qtree":

             bus: macio.0
               type macio-bus
               [...]
    +          dev: macio-nvram, id ""
    +            size = 8192 (0x2000)
    +            it_shift = 4 (0x4)

This also makes it a QOM child of macio-oldworld.  Visible in "info
qom-tree":

     /machine (g3beige-machine)
       [...]
       /unattached (container)
         [...]
         /device[6] (macio-oldworld)
           [...]
    -    /device[7] (macio-nvram)
    -      /macio-nvram[0] (qemu:memory-region)
    +      /nvram (macio-nvram)
    +        /macio-nvram[0] (qemu:memory-region)
         [rest of device[*] renumbered...]

Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/macio/macio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index b3dddf8be7..ebc96cc8f6 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -245,7 +245,8 @@ static void macio_oldworld_init(Object *obj)
 
     macio_init_child_obj(s, "cuda", &s->cuda, sizeof(s->cuda), TYPE_CUDA);
 
-    object_initialize(&os->nvram, sizeof(os->nvram), TYPE_MACIO_NVRAM);
+    macio_init_child_obj(s, "nvram", &os->nvram, sizeof(os->nvram),
+                         TYPE_MACIO_NVRAM);
     dev = DEVICE(&os->nvram);
     qdev_prop_set_uint32(dev, "size", 0x2000);
     qdev_prop_set_uint32(dev, "it_shift", 4);
-- 
2.21.1

Re: [PATCH 14/24] macio: Put "macio-nvram" device on the macio bus

[Mark Cave-Ayland]

On 18/05/2020 06:03, Markus Armbruster wrote:

> macio_oldworld_init() creates a "macio-nvram", sysbus device, but
> neglects to but it on a bus.
> 
> Put it on the macio bus.  Affects machine g3beige.  Visible in "info
> qtree":
> 
>              bus: macio.0
>                type macio-bus
>                [...]
>     +          dev: macio-nvram, id ""
>     +            size = 8192 (0x2000)
>     +            it_shift = 4 (0x4)
> 
> This also makes it a QOM child of macio-oldworld.  Visible in "info
> qom-tree":
> 
>      /machine (g3beige-machine)
>        [...]
>        /unattached (container)
>          [...]
>          /device[6] (macio-oldworld)
>            [...]
>     -    /device[7] (macio-nvram)
>     -      /macio-nvram[0] (qemu:memory-region)
>     +      /nvram (macio-nvram)
>     +        /macio-nvram[0] (qemu:memory-region)
>          [rest of device[*] renumbered...]
> 
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/macio/macio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index b3dddf8be7..ebc96cc8f6 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -245,7 +245,8 @@ static void macio_oldworld_init(Object *obj)
>  
>      macio_init_child_obj(s, "cuda", &s->cuda, sizeof(s->cuda), TYPE_CUDA);
>  
> -    object_initialize(&os->nvram, sizeof(os->nvram), TYPE_MACIO_NVRAM);
> +    macio_init_child_obj(s, "nvram", &os->nvram, sizeof(os->nvram),
> +                         TYPE_MACIO_NVRAM);
>      dev = DEVICE(&os->nvram);
>      qdev_prop_set_uint32(dev, "size", 0x2000);
>      qdev_prop_set_uint32(dev, "it_shift", 4);

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 15/24] macio: Fix macio-bus to be a subtype of System bus

[Markus Armbruster]

The devices we plug into the macio-bus are all sysbus devices
(DeviceClass member bus_type is TYPE_SYSTEM_BUS), but macio-bus does
not derive from TYPE_SYSTEM_BUS.  Fix that.

"info qtree" now shows the devices' mmio ranges, as it should

Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/macio/macio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index ebc96cc8f6..53a9fd5696 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -492,7 +492,7 @@ static void macio_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo macio_bus_info = {
     .name = TYPE_MACIO_BUS,
-    .parent = TYPE_BUS,
+    .parent = TYPE_SYSTEM_BUS,
     .instance_size = sizeof(MacIOBusState),
 };
 
-- 
2.21.1

Re: [PATCH 15/24] macio: Fix macio-bus to be a subtype of System bus

[Philippe Mathieu-Daudé]

On 5/18/20 7:03 AM, Markus Armbruster wrote:
> The devices we plug into the macio-bus are all sysbus devices
> (DeviceClass member bus_type is TYPE_SYSTEM_BUS), but macio-bus does
> not derive from TYPE_SYSTEM_BUS.  Fix that.
> 
> "info qtree" now shows the devices' mmio ranges, as it should
> 
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/misc/macio/macio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index ebc96cc8f6..53a9fd5696 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -492,7 +492,7 @@ static void macio_class_init(ObjectClass *klass, void *data)
>   
>   static const TypeInfo macio_bus_info = {
>       .name = TYPE_MACIO_BUS,
> -    .parent = TYPE_BUS,
> +    .parent = TYPE_SYSTEM_BUS,
>       .instance_size = sizeof(MacIOBusState),
>   };
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 15/24] macio: Fix macio-bus to be a subtype of System bus

[Mark Cave-Ayland]

On 18/05/2020 06:03, Markus Armbruster wrote:

> The devices we plug into the macio-bus are all sysbus devices
> (DeviceClass member bus_type is TYPE_SYSTEM_BUS), but macio-bus does
> not derive from TYPE_SYSTEM_BUS.  Fix that.
> 
> "info qtree" now shows the devices' mmio ranges, as it should
> 
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/macio/macio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index ebc96cc8f6..53a9fd5696 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -492,7 +492,7 @@ static void macio_class_init(ObjectClass *klass, void *data)
>  
>  static const TypeInfo macio_bus_info = {
>      .name = TYPE_MACIO_BUS,
> -    .parent = TYPE_BUS,
> +    .parent = TYPE_SYSTEM_BUS,
>      .instance_size = sizeof(MacIOBusState),
>  };

Here I learned something new: a device that has a class TYPE_SYS_BUS_DEVICE should be
attached to a bus that derives from TYPE_SYSTEM_BUS. I have a feeling that there are
going to be quite a few instances of this around, particularly in places where
existing sysbus devices have been borrowed from the PC world and reused.


ATB,

Mark.

Re: [PATCH 15/24] macio: Fix macio-bus to be a subtype of System bus

[Markus Armbruster]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> On 18/05/2020 06:03, Markus Armbruster wrote:
>
>> The devices we plug into the macio-bus are all sysbus devices
>> (DeviceClass member bus_type is TYPE_SYSTEM_BUS), but macio-bus does
>> not derive from TYPE_SYSTEM_BUS.  Fix that.
>> 
>> "info qtree" now shows the devices' mmio ranges, as it should
>> 
>> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Cc: qemu-ppc@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/misc/macio/macio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index ebc96cc8f6..53a9fd5696 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -492,7 +492,7 @@ static void macio_class_init(ObjectClass *klass, void *data)
>>  
>>  static const TypeInfo macio_bus_info = {
>>      .name = TYPE_MACIO_BUS,
>> -    .parent = TYPE_BUS,
>> +    .parent = TYPE_SYSTEM_BUS,
>>      .instance_size = sizeof(MacIOBusState),
>>  };
>
> Here I learned something new: a device that has a class TYPE_SYS_BUS_DEVICE should be
> attached to a bus that derives from TYPE_SYSTEM_BUS. I have a feeling that there are
> going to be quite a few instances of this around, particularly in places where
> existing sysbus devices have been borrowed from the PC world and reused.

Not that many.  I clean them up this series, and "[PATCH 22/24] qdev:
Assert devices are plugged into a bus that can take them" should ensure
we stay clean.

[PATCH 16/24] ppc/pnv: Put "*-pnv-chip" and "pnv-xive" on the main system bus

[Markus Armbruster]

pnv_init() creates "power10_v1.0-pnv-chip", "power8_v2.0-pnv-chip",
"power8e_v2.1-pnv-chip", "power8nvl_v1.0-pnv-chip", or
"power9_v2.0-pnv-chip" sysbus devices in a way that leaves them
unplugged.

pnv_chip_power9_instance_init() creates a "pnv-xive" sysbus device in
a way that leaves it unplugged.

Create them the common way that puts them into the main system bus.
Affects machines powernv8, powernv9, and powernv10.  Visible in "info
qtree".  Here's the change for powernv9:

     bus: main-system-bus
       type System
    +  dev: power9_v2.0-pnv-chip, id ""
    +    chip-id = 0 (0x0)
    +    ram-start = 0 (0x0)
    +    ram-size = 1879048192 (0x70000000)
    +    nr-cores = 1 (0x1)
    +    cores-mask = 72057594037927935 (0xffffffffffffff)
    +    nr-threads = 1 (0x1)
    +    num-phbs = 6 (0x6)
    +    mmio 000603fc00000000/0000000400000000
    [...]
    +  dev: pnv-xive, id ""
    +    ic-bar = 1692157036462080 (0x6030203100000)
    +    vc-bar = 1689949371891712 (0x6010000000000)
    +    pc-bar = 1690499127705600 (0x6018000000000)
    +    tm-bar = 1692157036986368 (0x6030203180000)

Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/pnv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index da637822f9..8d4fc8109a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -818,7 +818,7 @@ static void pnv_init(MachineState *machine)
     pnv->chips = g_new0(PnvChip *, pnv->num_chips);
     for (i = 0; i < pnv->num_chips; i++) {
         char chip_name[32];
-        Object *chip = object_new(chip_typename);
+        Object *chip = OBJECT(qdev_create(NULL, chip_typename));
 
         pnv->chips[i] = PNV_CHIP(chip);
 
@@ -1317,8 +1317,8 @@ static void pnv_chip_power9_instance_init(Object *obj)
     PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
     int i;
 
-    object_initialize_child(obj, "xive", &chip9->xive, sizeof(chip9->xive),
-                            TYPE_PNV_XIVE, &error_abort, NULL);
+    sysbus_init_child_obj(obj, "xive", &chip9->xive, sizeof(chip9->xive),
+                          TYPE_PNV_XIVE);
     object_property_add_alias(obj, "xive-fabric", OBJECT(&chip9->xive),
                               "xive-fabric");
 
-- 
2.21.1

Re: [PATCH 16/24] ppc/pnv: Put "*-pnv-chip" and "pnv-xive" on the main system bus

[Cédric Le Goater]

On 5/18/20 7:04 AM, Markus Armbruster wrote:
> pnv_init() creates "power10_v1.0-pnv-chip", "power8_v2.0-pnv-chip",
> "power8e_v2.1-pnv-chip", "power8nvl_v1.0-pnv-chip", or
> "power9_v2.0-pnv-chip" sysbus devices in a way that leaves them
> unplugged.
> 
> pnv_chip_power9_instance_init() creates a "pnv-xive" sysbus device in
> a way that leaves it unplugged.
> 
> Create them the common way that puts them into the main system bus.
> Affects machines powernv8, powernv9, and powernv10.  Visible in "info
> qtree".  Here's the change for powernv9:
> 
>      bus: main-system-bus
>        type System
>     +  dev: power9_v2.0-pnv-chip, id ""
>     +    chip-id = 0 (0x0)
>     +    ram-start = 0 (0x0)
>     +    ram-size = 1879048192 (0x70000000)
>     +    nr-cores = 1 (0x1)
>     +    cores-mask = 72057594037927935 (0xffffffffffffff)
>     +    nr-threads = 1 (0x1)
>     +    num-phbs = 6 (0x6)
>     +    mmio 000603fc00000000/0000000400000000
>     [...]
>     +  dev: pnv-xive, id ""
>     +    ic-bar = 1692157036462080 (0x6030203100000)
>     +    vc-bar = 1689949371891712 (0x6010000000000)
>     +    pc-bar = 1690499127705600 (0x6018000000000)
>     +    tm-bar = 1692157036986368 (0x6030203180000)
> 
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/pnv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index da637822f9..8d4fc8109a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -818,7 +818,7 @@ static void pnv_init(MachineState *machine)
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
> -        Object *chip = object_new(chip_typename);
> +        Object *chip = OBJECT(qdev_create(NULL, chip_typename));
>  
>          pnv->chips[i] = PNV_CHIP(chip);
>  
> @@ -1317,8 +1317,8 @@ static void pnv_chip_power9_instance_init(Object *obj)
>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
>      int i;
>  
> -    object_initialize_child(obj, "xive", &chip9->xive, sizeof(chip9->xive),
> -                            TYPE_PNV_XIVE, &error_abort, NULL);
> +    sysbus_init_child_obj(obj, "xive", &chip9->xive, sizeof(chip9->xive),
> +                          TYPE_PNV_XIVE);
>      object_property_add_alias(obj, "xive-fabric", OBJECT(&chip9->xive),
>                                "xive-fabric");

OK. But why only XIVE and not all sub-devices of the PnvChip device ? 

Shouldn't they be initialized in the same way, calling sysbus_init_child_obj ? 

Thanks,

C.
 

Re: [PATCH 16/24] ppc/pnv: Put "*-pnv-chip" and "pnv-xive" on the main system bus

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 5/18/20 7:04 AM, Markus Armbruster wrote:
>> pnv_init() creates "power10_v1.0-pnv-chip", "power8_v2.0-pnv-chip",
>> "power8e_v2.1-pnv-chip", "power8nvl_v1.0-pnv-chip", or
>> "power9_v2.0-pnv-chip" sysbus devices in a way that leaves them
>> unplugged.
>> 
>> pnv_chip_power9_instance_init() creates a "pnv-xive" sysbus device in
>> a way that leaves it unplugged.
>> 
>> Create them the common way that puts them into the main system bus.
>> Affects machines powernv8, powernv9, and powernv10.  Visible in "info
>> qtree".  Here's the change for powernv9:
>> 
>>      bus: main-system-bus
>>        type System
>>     +  dev: power9_v2.0-pnv-chip, id ""
>>     +    chip-id = 0 (0x0)
>>     +    ram-start = 0 (0x0)
>>     +    ram-size = 1879048192 (0x70000000)
>>     +    nr-cores = 1 (0x1)
>>     +    cores-mask = 72057594037927935 (0xffffffffffffff)
>>     +    nr-threads = 1 (0x1)
>>     +    num-phbs = 6 (0x6)
>>     +    mmio 000603fc00000000/0000000400000000
>>     [...]
>>     +  dev: pnv-xive, id ""
>>     +    ic-bar = 1692157036462080 (0x6030203100000)
>>     +    vc-bar = 1689949371891712 (0x6010000000000)
>>     +    pc-bar = 1690499127705600 (0x6018000000000)
>>     +    tm-bar = 1692157036986368 (0x6030203180000)
>> 
>> Cc: "Cédric Le Goater" <clg@kaod.org>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Cc: qemu-ppc@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ppc/pnv.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index da637822f9..8d4fc8109a 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -818,7 +818,7 @@ static void pnv_init(MachineState *machine)
>>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
>>      for (i = 0; i < pnv->num_chips; i++) {
>>          char chip_name[32];
>> -        Object *chip = object_new(chip_typename);
>> +        Object *chip = OBJECT(qdev_create(NULL, chip_typename));
>>  
>>          pnv->chips[i] = PNV_CHIP(chip);
>>  
>> @@ -1317,8 +1317,8 @@ static void pnv_chip_power9_instance_init(Object *obj)
>>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
>>      int i;
>>  
>> -    object_initialize_child(obj, "xive", &chip9->xive, sizeof(chip9->xive),
>> -                            TYPE_PNV_XIVE, &error_abort, NULL);
>> +    sysbus_init_child_obj(obj, "xive", &chip9->xive, sizeof(chip9->xive),
>> +                          TYPE_PNV_XIVE);
>>      object_property_add_alias(obj, "xive-fabric", OBJECT(&chip9->xive),
>>                                "xive-fabric");
>
> OK. But why only XIVE and not all sub-devices of the PnvChip device ? 
>
> Shouldn't they be initialized in the same way, calling sysbus_init_child_obj ? 
No, your code is just fine there.

sysbus_init_child_obj() is a convenience wrapper around
object_initialize_child() and qdev_set_parent_bus().  Only sysbus
devices may use it.  The other sub-devices are not susbus devices:

* TYPE_PNV8_PSI, TYPE_PNV9_PSI, TYPE_PNV10_PSI

  Subtypes of TYPE_PNV_PSI, which is a subtype of TYPE_DEVICE.

* TYPE_PNV8_LPC, TYPE_PNV9_LPC, TYPE_PNV10_LPC

  Subtypes of TYPE_PNV_LPC, which is a subtype of TYPE_DEVICE.

* TYPE_PNV8_OCC, TYPE_PNV9_OCC

  Subtypes of TYPE_PNV_OCC, which is a subtype of TYPE_DEVICE.

* TYPE_PNV8_HOMER, TYPE_PNV9_HOMER

  Subtypes of TYPE_PNV_HOMER, which is a subtype of TYPE_DEVICE.

* TYPE_PNV_PHB4_PEC

  Subtype of TYPE_DEVICE.

* TYPE_PNV_QUAD

  Subtype of TYPE_DEVICE.

Except for:

* TYPE_PNV_PHB3

  Subtype of TYPE_PCIE_HOST_BRIDGE, which is a subtype of
  TYPE_PCI_HOST_BRIDGE, which is a subtype of TYPE_SYS_BUS_DEVICE.

where you use object_initialize_child() and qdev_set_parent_bus()
directly.  Works.  We could perhaps change it to use
sysbus_init_child_obj(), but it would be a waste; my next series will
kill that helper :)

Re: [PATCH 16/24] ppc/pnv: Put "*-pnv-chip" and "pnv-xive" on the main system bus

[Cédric Le Goater]

On 5/18/20 7:04 AM, Markus Armbruster wrote:
> pnv_init() creates "power10_v1.0-pnv-chip", "power8_v2.0-pnv-chip",
> "power8e_v2.1-pnv-chip", "power8nvl_v1.0-pnv-chip", or
> "power9_v2.0-pnv-chip" sysbus devices in a way that leaves them
> unplugged.
> 
> pnv_chip_power9_instance_init() creates a "pnv-xive" sysbus device in
> a way that leaves it unplugged.
> 
> Create them the common way that puts them into the main system bus.
> Affects machines powernv8, powernv9, and powernv10.  Visible in "info
> qtree".  Here's the change for powernv9:
> 
>      bus: main-system-bus
>        type System
>     +  dev: power9_v2.0-pnv-chip, id ""
>     +    chip-id = 0 (0x0)
>     +    ram-start = 0 (0x0)
>     +    ram-size = 1879048192 (0x70000000)
>     +    nr-cores = 1 (0x1)
>     +    cores-mask = 72057594037927935 (0xffffffffffffff)
>     +    nr-threads = 1 (0x1)
>     +    num-phbs = 6 (0x6)
>     +    mmio 000603fc00000000/0000000400000000
>     [...]
>     +  dev: pnv-xive, id ""
>     +    ic-bar = 1692157036462080 (0x6030203100000)
>     +    vc-bar = 1689949371891712 (0x6010000000000)
>     +    pc-bar = 1690499127705600 (0x6018000000000)
>     +    tm-bar = 1692157036986368 (0x6030203180000)
> 
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

> ---
>  hw/ppc/pnv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index da637822f9..8d4fc8109a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -818,7 +818,7 @@ static void pnv_init(MachineState *machine)
>      pnv->chips = g_new0(PnvChip *, pnv->num_chips);
>      for (i = 0; i < pnv->num_chips; i++) {
>          char chip_name[32];
> -        Object *chip = object_new(chip_typename);
> +        Object *chip = OBJECT(qdev_create(NULL, chip_typename));
>  
>          pnv->chips[i] = PNV_CHIP(chip);
>  
> @@ -1317,8 +1317,8 @@ static void pnv_chip_power9_instance_init(Object *obj)
>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
>      int i;
>  
> -    object_initialize_child(obj, "xive", &chip9->xive, sizeof(chip9->xive),
> -                            TYPE_PNV_XIVE, &error_abort, NULL);
> +    sysbus_init_child_obj(obj, "xive", &chip9->xive, sizeof(chip9->xive),
> +                          TYPE_PNV_XIVE);
>      object_property_add_alias(obj, "xive-fabric", OBJECT(&chip9->xive),
>                                "xive-fabric");
>  
> 

[PATCH 17/24] pnv/psi: Correct the pnv-psi* devices not to be sysbus devices

[Markus Armbruster]

pnv_chip_power8_instance_init() creates a "pnv-psi-POWER8" sysbus
device in a way that leaves it unplugged.
pnv_chip_power9_instance_init() and pnv_chip_power10_instance_init()
do the same for "pnv-psi-POWER9" and "pnv-psi-POWER10", respectively.

These devices aren't actually sysbus devices.  Correct that.

Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/ppc/pnv_psi.h | 2 +-
 hw/ppc/pnv_psi.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/ppc/pnv_psi.h b/include/hw/ppc/pnv_psi.h
index f0f5b55197..979fc59f33 100644
--- a/include/hw/ppc/pnv_psi.h
+++ b/include/hw/ppc/pnv_psi.h
@@ -31,7 +31,7 @@
 #define PSIHB_XSCOM_MAX         0x20
 
 typedef struct PnvPsi {
-    SysBusDevice parent;
+    DeviceState parent;
 
     MemoryRegion regs_mr;
     uint64_t bar;
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index cfd5b7bc25..82f0769465 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -943,7 +943,7 @@ static void pnv_psi_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo pnv_psi_info = {
     .name          = TYPE_PNV_PSI,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_DEVICE,
     .instance_size = sizeof(PnvPsi),
     .class_init    = pnv_psi_class_init,
     .class_size    = sizeof(PnvPsiClass),
-- 
2.21.1

Re: [PATCH 17/24] pnv/psi: Correct the pnv-psi* devices not to be sysbus devices

[Cédric Le Goater]

On 5/18/20 7:04 AM, Markus Armbruster wrote:
> pnv_chip_power8_instance_init() creates a "pnv-psi-POWER8" sysbus
> device in a way that leaves it unplugged.
> pnv_chip_power9_instance_init() and pnv_chip_power10_instance_init()
> do the same for "pnv-psi-POWER9" and "pnv-psi-POWER10", respectively.
> 
> These devices aren't actually sysbus devices.  Correct that.

I might have done things wrong regarding sysbus in the PowerNV machine.

For some devices (PHBs), I have added :

	qdev_set_parent_bus(DEVICE(...), sysbus_get_default());

Should we do the same for the PSI device ?

Thanks,

C.

> 
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/hw/ppc/pnv_psi.h | 2 +-
>  hw/ppc/pnv_psi.c         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_psi.h b/include/hw/ppc/pnv_psi.h
> index f0f5b55197..979fc59f33 100644
> --- a/include/hw/ppc/pnv_psi.h
> +++ b/include/hw/ppc/pnv_psi.h
> @@ -31,7 +31,7 @@
>  #define PSIHB_XSCOM_MAX         0x20
>  
>  typedef struct PnvPsi {
> -    SysBusDevice parent;
> +    DeviceState parent;
>  
>      MemoryRegion regs_mr;
>      uint64_t bar;
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index cfd5b7bc25..82f0769465 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -943,7 +943,7 @@ static void pnv_psi_class_init(ObjectClass *klass, void *data)
>  
>  static const TypeInfo pnv_psi_info = {
>      .name          = TYPE_PNV_PSI,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .parent        = TYPE_DEVICE,
>      .instance_size = sizeof(PnvPsi),
>      .class_init    = pnv_psi_class_init,
>      .class_size    = sizeof(PnvPsiClass),
> 

Re: [PATCH 17/24] pnv/psi: Correct the pnv-psi* devices not to be sysbus devices

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 5/18/20 7:04 AM, Markus Armbruster wrote:
>> pnv_chip_power8_instance_init() creates a "pnv-psi-POWER8" sysbus
>> device in a way that leaves it unplugged.
>> pnv_chip_power9_instance_init() and pnv_chip_power10_instance_init()
>> do the same for "pnv-psi-POWER9" and "pnv-psi-POWER10", respectively.
>> 
>> These devices aren't actually sysbus devices.  Correct that.
>
> I might have done things wrong regarding sysbus in the PowerNV machine.
>
> For some devices (PHBs), I have added :
>
> 	qdev_set_parent_bus(DEVICE(...), sysbus_get_default());

It's not wrong.

My next series will rework how devices get plugged into their buses.

> Should we do the same for the PSI device ?

No, because the PSI device is not a sysbus device.

Re: [PATCH 17/24] pnv/psi: Correct the pnv-psi* devices not to be sysbus devices

[Cédric Le Goater]

On 5/18/20 7:04 AM, Markus Armbruster wrote:
> pnv_chip_power8_instance_init() creates a "pnv-psi-POWER8" sysbus
> device in a way that leaves it unplugged.
> pnv_chip_power9_instance_init() and pnv_chip_power10_instance_init()
> do the same for "pnv-psi-POWER9" and "pnv-psi-POWER10", respectively.
> 
> These devices aren't actually sysbus devices.  Correct that.
> 
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  include/hw/ppc/pnv_psi.h | 2 +-
>  hw/ppc/pnv_psi.c         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_psi.h b/include/hw/ppc/pnv_psi.h
> index f0f5b55197..979fc59f33 100644
> --- a/include/hw/ppc/pnv_psi.h
> +++ b/include/hw/ppc/pnv_psi.h
> @@ -31,7 +31,7 @@
>  #define PSIHB_XSCOM_MAX         0x20
>  
>  typedef struct PnvPsi {
> -    SysBusDevice parent;
> +    DeviceState parent;
>  
>      MemoryRegion regs_mr;
>      uint64_t bar;
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index cfd5b7bc25..82f0769465 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -943,7 +943,7 @@ static void pnv_psi_class_init(ObjectClass *klass, void *data)
>  
>  static const TypeInfo pnv_psi_info = {
>      .name          = TYPE_PNV_PSI,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .parent        = TYPE_DEVICE,
>      .instance_size = sizeof(PnvPsi),
>      .class_init    = pnv_psi_class_init,
>      .class_size    = sizeof(PnvPsiClass),
> 

[PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"

[Markus Armbruster]

sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
neglect to realize it.  Affects machines sam460ex, shix, r2d, and
fulong2e.

I wonder how this ever worked.  If the "device becomes real only on
realize" thing actually works, then we've always been missing the
device, yet nobody noticed.

Fix by realizing it right away.  Visible in "info qom-tree"; here's
the change for sam460ex:

     /machine (sam460ex-machine)
       [...]
       /unattached (container)
         [...]
    -    /device[14] (sii3112)
    +    /device[14] (i2c-ddc)
    +    /device[15] (sii3112)
         [rest of device[*] renumbered...]

Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
Cc: BALATON Zoltan <balaton@eik.bme.hu>
Cc: qemu-ppc@nongnu.org
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/display/ati.c   | 1 +
 hw/display/sm501.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 58ec8291d4..7c2177d7b3 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -929,6 +929,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
     bitbang_i2c_init(&s->bbi2c, i2cbus);
     I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
     i2c_set_slave_address(i2cddc, 0x50);
+    qdev_init_nofail(DEVICE(i2cddc));
 
     /* mmio register space */
     memory_region_init_io(&s->mm, OBJECT(s), &ati_mm_ops, s,
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index acc692531a..132e75b641 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1816,6 +1816,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
     /* ddc */
     I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
     i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
+    qdev_init_nofail(DEVICE(ddc));
 
     /* mmio */
     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
-- 
2.21.1

Re: [PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2544 bytes --]

On Mon, 18 May 2020, Markus Armbruster wrote:
> sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
> neglect to realize it.  Affects machines sam460ex, shix, r2d, and
> fulong2e.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing the
> device, yet nobody noticed.

No idea why it worked but guests can read EDID info fine with or without 
this patch, so

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

Maybe device is created and working after init as it has nothing special 
to do at realize (it doesn't even have a realize method) so all realize 
would do is to link it in qtree?

Regards,
BALATON Zoltan

> Fix by realizing it right away.  Visible in "info qom-tree"; here's
> the change for sam460ex:
>
>     /machine (sam460ex-machine)
>       [...]
>       /unattached (container)
>         [...]
>    -    /device[14] (sii3112)
>    +    /device[14] (i2c-ddc)
>    +    /device[15] (sii3112)
>         [rest of device[*] renumbered...]
>
> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: qemu-ppc@nongnu.org
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/display/ati.c   | 1 +
> hw/display/sm501.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 58ec8291d4..7c2177d7b3 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -929,6 +929,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
>     bitbang_i2c_init(&s->bbi2c, i2cbus);
>     I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
>     i2c_set_slave_address(i2cddc, 0x50);
> +    qdev_init_nofail(DEVICE(i2cddc));
>
>     /* mmio register space */
>     memory_region_init_io(&s->mm, OBJECT(s), &ati_mm_ops, s,
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index acc692531a..132e75b641 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1816,6 +1816,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>     /* ddc */
>     I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
>     i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
> +    qdev_init_nofail(DEVICE(ddc));
>
>     /* mmio */
>     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
>

Re: [PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Mon, 18 May 2020, Markus Armbruster wrote:
>> sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
>> neglect to realize it.  Affects machines sam460ex, shix, r2d, and
>> fulong2e.
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing the
>> device, yet nobody noticed.
>
> No idea why it worked but guests can read EDID info fine with or
> without this patch, so
>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

Thanks!

> Maybe device is created and working after init as it has nothing
> special to do at realize (it doesn't even have a realize method) so
> all realize would do is to link it in qtree?

Plausible.

Re: [PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

On Mon, 18 May 2020, Markus Armbruster wrote:
> sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
> neglect to realize it.  Affects machines sam460ex, shix, r2d, and
> fulong2e.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing the
> device, yet nobody noticed.
>
> Fix by realizing it right away.  Visible in "info qom-tree"; here's
> the change for sam460ex:
>
>     /machine (sam460ex-machine)
>       [...]
>       /unattached (container)
>         [...]
>    -    /device[14] (sii3112)
>    +    /device[14] (i2c-ddc)
>    +    /device[15] (sii3112)
>         [rest of device[*] renumbered...]
>
> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a

One of these is probably meant to be c82c7336de58876862e6b4dccbda29e9240fd388
although I'm not sure having a Fixes tag makes sense for this commit.

> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: qemu-ppc@nongnu.org
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/display/ati.c   | 1 +
> hw/display/sm501.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 58ec8291d4..7c2177d7b3 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -929,6 +929,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
>     bitbang_i2c_init(&s->bbi2c, i2cbus);
>     I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
>     i2c_set_slave_address(i2cddc, 0x50);
> +    qdev_init_nofail(DEVICE(i2cddc));
>
>     /* mmio register space */
>     memory_region_init_io(&s->mm, OBJECT(s), &ati_mm_ops, s,
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index acc692531a..132e75b641 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1816,6 +1816,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>     /* ddc */
>     I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
>     i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
> +    qdev_init_nofail(DEVICE(ddc));
>
>     /* mmio */
>     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
>

Re: [PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"

[Philippe Mathieu-Daudé]

On 5/18/20 12:39 PM, BALATON Zoltan wrote:
> On Mon, 18 May 2020, Markus Armbruster wrote:
>> sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
>> neglect to realize it.  Affects machines sam460ex, shix, r2d, and
>> fulong2e.
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing the
>> device, yet nobody noticed.
>>
>> Fix by realizing it right away.  Visible in "info qom-tree"; here's
>> the change for sam460ex:
>>
>>     /machine (sam460ex-machine)
>>       [...]
>>       /unattached (container)
>>         [...]
>>    -    /device[14] (sii3112)
>>    +    /device[14] (i2c-ddc)
>>    +    /device[15] (sii3112)
>>         [rest of device[*] renumbered...]
>>
>> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
>> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
> 
> One of these is probably meant to be 
> c82c7336de58876862e6b4dccbda29e9240fd388

:)

> although I'm not sure having a Fixes tag makes sense for this commit.

AFAIK the 'Fixes' tag is not well defined in QEMU.
I personally find it handy to navigate between commits in gitk, not 
having to go via unrelated commits, which is why I use it.
Linux kernel seems to have a stricter approach, only using it for 
security bug fixes. For this QEMU uses 'Cc: qemu-stable'.

Do we need to clarify its use on the wiki?

> 
>> Cc: BALATON Zoltan <balaton@eik.bme.hu>
>> Cc: qemu-ppc@nongnu.org
>> Cc: Magnus Damm <magnus.damm@gmail.com>
>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> hw/display/ati.c   | 1 +
>> hw/display/sm501.c | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>> index 58ec8291d4..7c2177d7b3 100644
>> --- a/hw/display/ati.c
>> +++ b/hw/display/ati.c
>> @@ -929,6 +929,7 @@ static void ati_vga_realize(PCIDevice *dev, Error 
>> **errp)
>>     bitbang_i2c_init(&s->bbi2c, i2cbus);
>>     I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
>>     i2c_set_slave_address(i2cddc, 0x50);
>> +    qdev_init_nofail(DEVICE(i2cddc));
>>
>>     /* mmio register space */
>>     memory_region_init_io(&s->mm, OBJECT(s), &ati_mm_ops, s,
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index acc692531a..132e75b641 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -1816,6 +1816,7 @@ static void sm501_init(SM501State *s, 
>> DeviceState *dev,
>>     /* ddc */
>>     I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
>>     i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
>> +    qdev_init_nofail(DEVICE(ddc));
>>
>>     /* mmio */
>>     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", 
>> MMIO_SIZE);
>>

Re: [PATCH 18/24] display/sm501 display/ati: Fix to realize "i2c-ddc"

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 5/18/20 12:39 PM, BALATON Zoltan wrote:
>> On Mon, 18 May 2020, Markus Armbruster wrote:
>>> sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
>>> neglect to realize it.  Affects machines sam460ex, shix, r2d, and
>>> fulong2e.
>>>
>>> I wonder how this ever worked.  If the "device becomes real only on
>>> realize" thing actually works, then we've always been missing the
>>> device, yet nobody noticed.
>>>
>>> Fix by realizing it right away.  Visible in "info qom-tree"; here's
>>> the change for sam460ex:
>>>
>>>     /machine (sam460ex-machine)
>>>       [...]
>>>       /unattached (container)
>>>         [...]
>>>    -    /device[14] (sii3112)
>>>    +    /device[14] (i2c-ddc)
>>>    +    /device[15] (sii3112)
>>>         [rest of device[*] renumbered...]
>>>
>>> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
>>> Fixes: 4a1f253adb45ac6019971193d5077c4d5d55886a
>>
>> One of these is probably meant to be
>> c82c7336de58876862e6b4dccbda29e9240fd388

Pasto, thanks!

> :)
>
>> although I'm not sure having a Fixes tag makes sense for this commit.
>
> AFAIK the 'Fixes' tag is not well defined in QEMU.

True.

> I personally find it handy to navigate between commits in gitk, not
> having to go via unrelated commits, which is why I use it.
> Linux kernel seems to have a stricter approach, only using it for
> security bug fixes. For this QEMU uses 'Cc: qemu-stable'.

We cc: qemu-stable for show-stoppers without security impact, too.

> Do we need to clarify its use on the wiki?

If we can build rough consensus on how we want it used, yes.

[PATCH 19/24] riscv: Fix to put "riscv.hart_array" devices on sysbus

[Markus Armbruster]

riscv_sifive_e_soc_init(), riscv_sifive_u_soc_init(),
spike_board_init(), spike_v1_10_0_board_init(),
spike_v1_09_1_board_init(), and riscv_virt_board_init() create
"riscv-hart_array" sysbus devices in a way that leaves them unplugged.

Create them the common way that puts them into the main system bus.
Affects machines sifive_e, sifive_u, spike, spike_v1.10, spike_v1.9.1,
and virt.  Visible in "info qtree", here's the change for sifive_e:

     bus: main-system-bus
       type System
    +  dev: riscv.hart_array, id ""
    +    num-harts = 1 (0x1)
    +    hartid-base = 0 (0x0)
    +    cpu-type = "sifive-e31-riscv-cpu"
       dev: sifive_soc.gpio, id ""

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/riscv/sifive_e.c |  5 ++---
 hw/riscv/sifive_u.c | 14 ++++++--------
 hw/riscv/spike.c    | 12 ++++++------
 hw/riscv/virt.c     |  4 ++--
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index b53109521e..8831e6728e 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -120,9 +120,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
     MachineState *ms = MACHINE(qdev_get_machine());
     SiFiveESoCState *s = RISCV_E_SOC(obj);
 
-    object_initialize_child(obj, "cpus", &s->cpus,
-                            sizeof(s->cpus), TYPE_RISCV_HART_ARRAY,
-                            &error_abort, NULL);
+    sysbus_init_child_obj(obj, "cpus", &s->cpus,
+                          sizeof(s->cpus), TYPE_RISCV_HART_ARRAY);
     object_property_set_int(OBJECT(&s->cpus), ms->smp.cpus, "num-harts",
                             &error_abort);
     sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 4299bdf480..bb69fd8e48 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -491,10 +491,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
                             &error_abort, NULL);
     qdev_prop_set_uint32(DEVICE(&s->e_cluster), "cluster-id", 0);
 
-    object_initialize_child(OBJECT(&s->e_cluster), "e-cpus",
-                            &s->e_cpus, sizeof(s->e_cpus),
-                            TYPE_RISCV_HART_ARRAY, &error_abort,
-                            NULL);
+    sysbus_init_child_obj(OBJECT(&s->e_cluster), "e-cpus",
+                          &s->e_cpus, sizeof(s->e_cpus),
+                          TYPE_RISCV_HART_ARRAY);
     qdev_prop_set_uint32(DEVICE(&s->e_cpus), "num-harts", 1);
     qdev_prop_set_uint32(DEVICE(&s->e_cpus), "hartid-base", 0);
     qdev_prop_set_string(DEVICE(&s->e_cpus), "cpu-type", SIFIVE_E_CPU);
@@ -504,10 +503,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
                             &error_abort, NULL);
     qdev_prop_set_uint32(DEVICE(&s->u_cluster), "cluster-id", 1);
 
-    object_initialize_child(OBJECT(&s->u_cluster), "u-cpus",
-                            &s->u_cpus, sizeof(s->u_cpus),
-                            TYPE_RISCV_HART_ARRAY, &error_abort,
-                            NULL);
+    sysbus_init_child_obj(OBJECT(&s->u_cluster), "u-cpus",
+                          &s->u_cpus, sizeof(s->u_cpus),
+                          TYPE_RISCV_HART_ARRAY);
     qdev_prop_set_uint32(DEVICE(&s->u_cpus), "num-harts", ms->smp.cpus - 1);
     qdev_prop_set_uint32(DEVICE(&s->u_cpus), "hartid-base", 1);
     qdev_prop_set_string(DEVICE(&s->u_cpus), "cpu-type", SIFIVE_U_CPU);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index d0c4843712..01d52e758e 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -169,8 +169,8 @@ static void spike_board_init(MachineState *machine)
     unsigned int smp_cpus = machine->smp.cpus;
 
     /* Initialize SOC */
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                          TYPE_RISCV_HART_ARRAY);
     object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
@@ -275,8 +275,8 @@ static void spike_v1_10_0_board_init(MachineState *machine)
     }
 
     /* Initialize SOC */
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                          TYPE_RISCV_HART_ARRAY);
     object_property_set_str(OBJECT(&s->soc), SPIKE_V1_10_0_CPU, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
@@ -365,8 +365,8 @@ static void spike_v1_09_1_board_init(MachineState *machine)
     }
 
     /* Initialize SOC */
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                          TYPE_RISCV_HART_ARRAY);
     object_property_set_str(OBJECT(&s->soc), SPIKE_V1_09_1_CPU, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index c695a44979..0f93e0d9c8 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -485,8 +485,8 @@ static void riscv_virt_board_init(MachineState *machine)
     unsigned int smp_cpus = machine->smp.cpus;
 
     /* Initialize SOC */
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                          TYPE_RISCV_HART_ARRAY);
     object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
-- 
2.21.1

[PATCH 19/24] riscv: Fix to put "riscv.hart_array" devices on sysbus

[Markus Armbruster]

riscv_sifive_e_soc_init(), riscv_sifive_u_soc_init(),
spike_board_init(), spike_v1_10_0_board_init(),
spike_v1_09_1_board_init(), and riscv_virt_board_init() create
"riscv-hart_array" sysbus devices in a way that leaves them unplugged.

Create them the common way that puts them into the main system bus.
Affects machines sifive_e, sifive_u, spike, spike_v1.10, spike_v1.9.1,
and virt.  Visible in "info qtree", here's the change for sifive_e:

     bus: main-system-bus
       type System
    +  dev: riscv.hart_array, id ""
    +    num-harts = 1 (0x1)
    +    hartid-base = 0 (0x0)
    +    cpu-type = "sifive-e31-riscv-cpu"
       dev: sifive_soc.gpio, id ""

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/riscv/sifive_e.c |  5 ++---
 hw/riscv/sifive_u.c | 14 ++++++--------
 hw/riscv/spike.c    | 12 ++++++------
 hw/riscv/virt.c     |  4 ++--
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index b53109521e..8831e6728e 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -120,9 +120,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
     MachineState *ms = MACHINE(qdev_get_machine());
     SiFiveESoCState *s = RISCV_E_SOC(obj);
 
-    object_initialize_child(obj, "cpus", &s->cpus,
-                            sizeof(s->cpus), TYPE_RISCV_HART_ARRAY,
-                            &error_abort, NULL);
+    sysbus_init_child_obj(obj, "cpus", &s->cpus,
+                          sizeof(s->cpus), TYPE_RISCV_HART_ARRAY);
     object_property_set_int(OBJECT(&s->cpus), ms->smp.cpus, "num-harts",
                             &error_abort);
     sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 4299bdf480..bb69fd8e48 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -491,10 +491,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
                             &error_abort, NULL);
     qdev_prop_set_uint32(DEVICE(&s->e_cluster), "cluster-id", 0);
 
-    object_initialize_child(OBJECT(&s->e_cluster), "e-cpus",
-                            &s->e_cpus, sizeof(s->e_cpus),
-                            TYPE_RISCV_HART_ARRAY, &error_abort,
-                            NULL);
+    sysbus_init_child_obj(OBJECT(&s->e_cluster), "e-cpus",
+                          &s->e_cpus, sizeof(s->e_cpus),
+                          TYPE_RISCV_HART_ARRAY);
     qdev_prop_set_uint32(DEVICE(&s->e_cpus), "num-harts", 1);
     qdev_prop_set_uint32(DEVICE(&s->e_cpus), "hartid-base", 0);
     qdev_prop_set_string(DEVICE(&s->e_cpus), "cpu-type", SIFIVE_E_CPU);
@@ -504,10 +503,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
                             &error_abort, NULL);
     qdev_prop_set_uint32(DEVICE(&s->u_cluster), "cluster-id", 1);
 
-    object_initialize_child(OBJECT(&s->u_cluster), "u-cpus",
-                            &s->u_cpus, sizeof(s->u_cpus),
-                            TYPE_RISCV_HART_ARRAY, &error_abort,
-                            NULL);
+    sysbus_init_child_obj(OBJECT(&s->u_cluster), "u-cpus",
+                          &s->u_cpus, sizeof(s->u_cpus),
+                          TYPE_RISCV_HART_ARRAY);
     qdev_prop_set_uint32(DEVICE(&s->u_cpus), "num-harts", ms->smp.cpus - 1);
     qdev_prop_set_uint32(DEVICE(&s->u_cpus), "hartid-base", 1);
     qdev_prop_set_string(DEVICE(&s->u_cpus), "cpu-type", SIFIVE_U_CPU);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index d0c4843712..01d52e758e 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -169,8 +169,8 @@ static void spike_board_init(MachineState *machine)
     unsigned int smp_cpus = machine->smp.cpus;
 
     /* Initialize SOC */
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                          TYPE_RISCV_HART_ARRAY);
     object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
@@ -275,8 +275,8 @@ static void spike_v1_10_0_board_init(MachineState *machine)
     }
 
     /* Initialize SOC */
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                          TYPE_RISCV_HART_ARRAY);
     object_property_set_str(OBJECT(&s->soc), SPIKE_V1_10_0_CPU, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
@@ -365,8 +365,8 @@ static void spike_v1_09_1_board_init(MachineState *machine)
     }
 
     /* Initialize SOC */
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                          TYPE_RISCV_HART_ARRAY);
     object_property_set_str(OBJECT(&s->soc), SPIKE_V1_09_1_CPU, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index c695a44979..0f93e0d9c8 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -485,8 +485,8 @@ static void riscv_virt_board_init(MachineState *machine)
     unsigned int smp_cpus = machine->smp.cpus;
 
     /* Initialize SOC */
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
+    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                          TYPE_RISCV_HART_ARRAY);
     object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
-- 
2.21.1

Re: [PATCH 19/24] riscv: Fix to put "riscv.hart_array" devices on sysbus

[Alistair Francis]

On Sun, May 17, 2020 at 10:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> riscv_sifive_e_soc_init(), riscv_sifive_u_soc_init(),
> spike_board_init(), spike_v1_10_0_board_init(),
> spike_v1_09_1_board_init(), and riscv_virt_board_init() create
> "riscv-hart_array" sysbus devices in a way that leaves them unplugged.
>
> Create them the common way that puts them into the main system bus.
> Affects machines sifive_e, sifive_u, spike, spike_v1.10, spike_v1.9.1,
> and virt.  Visible in "info qtree", here's the change for sifive_e:
>
>      bus: main-system-bus
>        type System
>     +  dev: riscv.hart_array, id ""
>     +    num-harts = 1 (0x1)
>     +    hartid-base = 0 (0x0)
>     +    cpu-type = "sifive-e31-riscv-cpu"
>        dev: sifive_soc.gpio, id ""
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/sifive_e.c |  5 ++---
>  hw/riscv/sifive_u.c | 14 ++++++--------
>  hw/riscv/spike.c    | 12 ++++++------
>  hw/riscv/virt.c     |  4 ++--
>  4 files changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index b53109521e..8831e6728e 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -120,9 +120,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
>      MachineState *ms = MACHINE(qdev_get_machine());
>      SiFiveESoCState *s = RISCV_E_SOC(obj);
>
> -    object_initialize_child(obj, "cpus", &s->cpus,
> -                            sizeof(s->cpus), TYPE_RISCV_HART_ARRAY,
> -                            &error_abort, NULL);
> +    sysbus_init_child_obj(obj, "cpus", &s->cpus,
> +                          sizeof(s->cpus), TYPE_RISCV_HART_ARRAY);
>      object_property_set_int(OBJECT(&s->cpus), ms->smp.cpus, "num-harts",
>                              &error_abort);
>      sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 4299bdf480..bb69fd8e48 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -491,10 +491,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
>                              &error_abort, NULL);
>      qdev_prop_set_uint32(DEVICE(&s->e_cluster), "cluster-id", 0);
>
> -    object_initialize_child(OBJECT(&s->e_cluster), "e-cpus",
> -                            &s->e_cpus, sizeof(s->e_cpus),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort,
> -                            NULL);
> +    sysbus_init_child_obj(OBJECT(&s->e_cluster), "e-cpus",
> +                          &s->e_cpus, sizeof(s->e_cpus),
> +                          TYPE_RISCV_HART_ARRAY);
>      qdev_prop_set_uint32(DEVICE(&s->e_cpus), "num-harts", 1);
>      qdev_prop_set_uint32(DEVICE(&s->e_cpus), "hartid-base", 0);
>      qdev_prop_set_string(DEVICE(&s->e_cpus), "cpu-type", SIFIVE_E_CPU);
> @@ -504,10 +503,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
>                              &error_abort, NULL);
>      qdev_prop_set_uint32(DEVICE(&s->u_cluster), "cluster-id", 1);
>
> -    object_initialize_child(OBJECT(&s->u_cluster), "u-cpus",
> -                            &s->u_cpus, sizeof(s->u_cpus),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort,
> -                            NULL);
> +    sysbus_init_child_obj(OBJECT(&s->u_cluster), "u-cpus",
> +                          &s->u_cpus, sizeof(s->u_cpus),
> +                          TYPE_RISCV_HART_ARRAY);
>      qdev_prop_set_uint32(DEVICE(&s->u_cpus), "num-harts", ms->smp.cpus - 1);
>      qdev_prop_set_uint32(DEVICE(&s->u_cpus), "hartid-base", 1);
>      qdev_prop_set_string(DEVICE(&s->u_cpus), "cpu-type", SIFIVE_U_CPU);
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index d0c4843712..01d52e758e 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -169,8 +169,8 @@ static void spike_board_init(MachineState *machine)
>      unsigned int smp_cpus = machine->smp.cpus;
>
>      /* Initialize SOC */
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                          TYPE_RISCV_HART_ARRAY);
>      object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> @@ -275,8 +275,8 @@ static void spike_v1_10_0_board_init(MachineState *machine)
>      }
>
>      /* Initialize SOC */
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                          TYPE_RISCV_HART_ARRAY);
>      object_property_set_str(OBJECT(&s->soc), SPIKE_V1_10_0_CPU, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> @@ -365,8 +365,8 @@ static void spike_v1_09_1_board_init(MachineState *machine)
>      }
>
>      /* Initialize SOC */
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                          TYPE_RISCV_HART_ARRAY);
>      object_property_set_str(OBJECT(&s->soc), SPIKE_V1_09_1_CPU, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index c695a44979..0f93e0d9c8 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -485,8 +485,8 @@ static void riscv_virt_board_init(MachineState *machine)
>      unsigned int smp_cpus = machine->smp.cpus;
>
>      /* Initialize SOC */
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                          TYPE_RISCV_HART_ARRAY);
>      object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> --
> 2.21.1
>
>

Re: [PATCH 19/24] riscv: Fix to put "riscv.hart_array" devices on sysbus

[Alistair Francis]

On Sun, May 17, 2020 at 10:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> riscv_sifive_e_soc_init(), riscv_sifive_u_soc_init(),
> spike_board_init(), spike_v1_10_0_board_init(),
> spike_v1_09_1_board_init(), and riscv_virt_board_init() create
> "riscv-hart_array" sysbus devices in a way that leaves them unplugged.
>
> Create them the common way that puts them into the main system bus.
> Affects machines sifive_e, sifive_u, spike, spike_v1.10, spike_v1.9.1,
> and virt.  Visible in "info qtree", here's the change for sifive_e:
>
>      bus: main-system-bus
>        type System
>     +  dev: riscv.hart_array, id ""
>     +    num-harts = 1 (0x1)
>     +    hartid-base = 0 (0x0)
>     +    cpu-type = "sifive-e31-riscv-cpu"
>        dev: sifive_soc.gpio, id ""
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/sifive_e.c |  5 ++---
>  hw/riscv/sifive_u.c | 14 ++++++--------
>  hw/riscv/spike.c    | 12 ++++++------
>  hw/riscv/virt.c     |  4 ++--
>  4 files changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index b53109521e..8831e6728e 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -120,9 +120,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
>      MachineState *ms = MACHINE(qdev_get_machine());
>      SiFiveESoCState *s = RISCV_E_SOC(obj);
>
> -    object_initialize_child(obj, "cpus", &s->cpus,
> -                            sizeof(s->cpus), TYPE_RISCV_HART_ARRAY,
> -                            &error_abort, NULL);
> +    sysbus_init_child_obj(obj, "cpus", &s->cpus,
> +                          sizeof(s->cpus), TYPE_RISCV_HART_ARRAY);
>      object_property_set_int(OBJECT(&s->cpus), ms->smp.cpus, "num-harts",
>                              &error_abort);
>      sysbus_init_child_obj(obj, "riscv.sifive.e.gpio0",
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 4299bdf480..bb69fd8e48 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -491,10 +491,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
>                              &error_abort, NULL);
>      qdev_prop_set_uint32(DEVICE(&s->e_cluster), "cluster-id", 0);
>
> -    object_initialize_child(OBJECT(&s->e_cluster), "e-cpus",
> -                            &s->e_cpus, sizeof(s->e_cpus),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort,
> -                            NULL);
> +    sysbus_init_child_obj(OBJECT(&s->e_cluster), "e-cpus",
> +                          &s->e_cpus, sizeof(s->e_cpus),
> +                          TYPE_RISCV_HART_ARRAY);
>      qdev_prop_set_uint32(DEVICE(&s->e_cpus), "num-harts", 1);
>      qdev_prop_set_uint32(DEVICE(&s->e_cpus), "hartid-base", 0);
>      qdev_prop_set_string(DEVICE(&s->e_cpus), "cpu-type", SIFIVE_E_CPU);
> @@ -504,10 +503,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
>                              &error_abort, NULL);
>      qdev_prop_set_uint32(DEVICE(&s->u_cluster), "cluster-id", 1);
>
> -    object_initialize_child(OBJECT(&s->u_cluster), "u-cpus",
> -                            &s->u_cpus, sizeof(s->u_cpus),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort,
> -                            NULL);
> +    sysbus_init_child_obj(OBJECT(&s->u_cluster), "u-cpus",
> +                          &s->u_cpus, sizeof(s->u_cpus),
> +                          TYPE_RISCV_HART_ARRAY);
>      qdev_prop_set_uint32(DEVICE(&s->u_cpus), "num-harts", ms->smp.cpus - 1);
>      qdev_prop_set_uint32(DEVICE(&s->u_cpus), "hartid-base", 1);
>      qdev_prop_set_string(DEVICE(&s->u_cpus), "cpu-type", SIFIVE_U_CPU);
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index d0c4843712..01d52e758e 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -169,8 +169,8 @@ static void spike_board_init(MachineState *machine)
>      unsigned int smp_cpus = machine->smp.cpus;
>
>      /* Initialize SOC */
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                          TYPE_RISCV_HART_ARRAY);
>      object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> @@ -275,8 +275,8 @@ static void spike_v1_10_0_board_init(MachineState *machine)
>      }
>
>      /* Initialize SOC */
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                          TYPE_RISCV_HART_ARRAY);
>      object_property_set_str(OBJECT(&s->soc), SPIKE_V1_10_0_CPU, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> @@ -365,8 +365,8 @@ static void spike_v1_09_1_board_init(MachineState *machine)
>      }
>
>      /* Initialize SOC */
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                          TYPE_RISCV_HART_ARRAY);
>      object_property_set_str(OBJECT(&s->soc), SPIKE_V1_09_1_CPU, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index c695a44979..0f93e0d9c8 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -485,8 +485,8 @@ static void riscv_virt_board_init(MachineState *machine)
>      unsigned int smp_cpus = machine->smp.cpus;
>
>      /* Initialize SOC */
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            TYPE_RISCV_HART_ARRAY, &error_abort, NULL);
> +    sysbus_init_child_obj(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                          TYPE_RISCV_HART_ARRAY);
>      object_property_set_str(OBJECT(&s->soc), machine->cpu_type, "cpu-type",
>                              &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "num-harts",
> --
> 2.21.1
>
>

[PATCH 20/24] riscv: Fix type of SiFive[EU]SocState, member parent_obj

[Markus Armbruster]

Device "riscv.sifive.e.soc" is a direct subtype of TYPE_DEVICE, but
its instance struct SiFiveESoCState's member @parent_obj is
SysBusDevice instead of DeviceState.  Correct that.

Same for "riscv.sifive.u.soc"'s instance struct SiFiveUSoCState.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/riscv/sifive_e.h | 2 +-
 include/hw/riscv/sifive_u.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
index 25ce7aa9d5..f05644df7c 100644
--- a/include/hw/riscv/sifive_e.h
+++ b/include/hw/riscv/sifive_e.h
@@ -29,7 +29,7 @@
 
 typedef struct SiFiveESoCState {
     /*< private >*/
-    SysBusDevice parent_obj;
+    DeviceState parent_obj;
 
     /*< public >*/
     RISCVHartArrayState cpus;
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 16c297ec5f..5f62cf5f85 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -31,7 +31,7 @@
 
 typedef struct SiFiveUSoCState {
     /*< private >*/
-    SysBusDevice parent_obj;
+    DeviceState parent_obj;
 
     /*< public >*/
     CPUClusterState e_cluster;
-- 
2.21.1

[PATCH 20/24] riscv: Fix type of SiFive[EU]SocState, member parent_obj

[Markus Armbruster]

Device "riscv.sifive.e.soc" is a direct subtype of TYPE_DEVICE, but
its instance struct SiFiveESoCState's member @parent_obj is
SysBusDevice instead of DeviceState.  Correct that.

Same for "riscv.sifive.u.soc"'s instance struct SiFiveUSoCState.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/riscv/sifive_e.h | 2 +-
 include/hw/riscv/sifive_u.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
index 25ce7aa9d5..f05644df7c 100644
--- a/include/hw/riscv/sifive_e.h
+++ b/include/hw/riscv/sifive_e.h
@@ -29,7 +29,7 @@
 
 typedef struct SiFiveESoCState {
     /*< private >*/
-    SysBusDevice parent_obj;
+    DeviceState parent_obj;
 
     /*< public >*/
     RISCVHartArrayState cpus;
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 16c297ec5f..5f62cf5f85 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -31,7 +31,7 @@
 
 typedef struct SiFiveUSoCState {
     /*< private >*/
-    SysBusDevice parent_obj;
+    DeviceState parent_obj;
 
     /*< public >*/
     CPUClusterState e_cluster;
-- 
2.21.1

Re: [PATCH 20/24] riscv: Fix type of SiFive[EU]SocState, member parent_obj

[Philippe Mathieu-Daudé]

On 5/18/20 7:04 AM, Markus Armbruster wrote:
> Device "riscv.sifive.e.soc" is a direct subtype of TYPE_DEVICE, but
> its instance struct SiFiveESoCState's member @parent_obj is
> SysBusDevice instead of DeviceState.  Correct that.
> 
> Same for "riscv.sifive.u.soc"'s instance struct SiFiveUSoCState.
> 
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/hw/riscv/sifive_e.h | 2 +-
>   include/hw/riscv/sifive_u.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 25ce7aa9d5..f05644df7c 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -29,7 +29,7 @@
>   
>   typedef struct SiFiveESoCState {
>       /*< private >*/
> -    SysBusDevice parent_obj;
> +    DeviceState parent_obj;
>   
>       /*< public >*/
>       RISCVHartArrayState cpus;
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 16c297ec5f..5f62cf5f85 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -31,7 +31,7 @@
>   
>   typedef struct SiFiveUSoCState {
>       /*< private >*/
> -    SysBusDevice parent_obj;
> +    DeviceState parent_obj;
>   
>       /*< public >*/
>       CPUClusterState e_cluster;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 20/24] riscv: Fix type of SiFive[EU]SocState, member parent_obj

[Philippe Mathieu-Daudé]

On 5/18/20 7:04 AM, Markus Armbruster wrote:
> Device "riscv.sifive.e.soc" is a direct subtype of TYPE_DEVICE, but
> its instance struct SiFiveESoCState's member @parent_obj is
> SysBusDevice instead of DeviceState.  Correct that.
> 
> Same for "riscv.sifive.u.soc"'s instance struct SiFiveUSoCState.
> 
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/hw/riscv/sifive_e.h | 2 +-
>   include/hw/riscv/sifive_u.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 25ce7aa9d5..f05644df7c 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -29,7 +29,7 @@
>   
>   typedef struct SiFiveESoCState {
>       /*< private >*/
> -    SysBusDevice parent_obj;
> +    DeviceState parent_obj;
>   
>       /*< public >*/
>       RISCVHartArrayState cpus;
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 16c297ec5f..5f62cf5f85 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -31,7 +31,7 @@
>   
>   typedef struct SiFiveUSoCState {
>       /*< private >*/
> -    SysBusDevice parent_obj;
> +    DeviceState parent_obj;
>   
>       /*< public >*/
>       CPUClusterState e_cluster;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 20/24] riscv: Fix type of SiFive[EU]SocState, member parent_obj

[Alistair Francis]

On Sun, May 17, 2020 at 10:07 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Device "riscv.sifive.e.soc" is a direct subtype of TYPE_DEVICE, but
> its instance struct SiFiveESoCState's member @parent_obj is
> SysBusDevice instead of DeviceState.  Correct that.
>
> Same for "riscv.sifive.u.soc"'s instance struct SiFiveUSoCState.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/riscv/sifive_e.h | 2 +-
>  include/hw/riscv/sifive_u.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 25ce7aa9d5..f05644df7c 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -29,7 +29,7 @@
>
>  typedef struct SiFiveESoCState {
>      /*< private >*/
> -    SysBusDevice parent_obj;
> +    DeviceState parent_obj;
>
>      /*< public >*/
>      RISCVHartArrayState cpus;
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 16c297ec5f..5f62cf5f85 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -31,7 +31,7 @@
>
>  typedef struct SiFiveUSoCState {
>      /*< private >*/
> -    SysBusDevice parent_obj;
> +    DeviceState parent_obj;
>
>      /*< public >*/
>      CPUClusterState e_cluster;
> --
> 2.21.1
>
>

Re: [PATCH 20/24] riscv: Fix type of SiFive[EU]SocState, member parent_obj

[Alistair Francis]

On Sun, May 17, 2020 at 10:07 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Device "riscv.sifive.e.soc" is a direct subtype of TYPE_DEVICE, but
> its instance struct SiFiveESoCState's member @parent_obj is
> SysBusDevice instead of DeviceState.  Correct that.
>
> Same for "riscv.sifive.u.soc"'s instance struct SiFiveUSoCState.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/riscv/sifive_e.h | 2 +-
>  include/hw/riscv/sifive_u.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 25ce7aa9d5..f05644df7c 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -29,7 +29,7 @@
>
>  typedef struct SiFiveESoCState {
>      /*< private >*/
> -    SysBusDevice parent_obj;
> +    DeviceState parent_obj;
>
>      /*< public >*/
>      RISCVHartArrayState cpus;
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 16c297ec5f..5f62cf5f85 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -31,7 +31,7 @@
>
>  typedef struct SiFiveUSoCState {
>      /*< private >*/
> -    SysBusDevice parent_obj;
> +    DeviceState parent_obj;
>
>      /*< public >*/
>      CPUClusterState e_cluster;
> --
> 2.21.1
>
>

[PATCH 21/24] sparc/leon3: Fix to put grlib,* devices on sysbus

[Markus Armbruster]

leon3_generic_hw_init() creates a "grlib,ahbpnp" and a "grlib,apbpnp"
sysbus device in a way that leaves them unplugged.

Create them the common way that puts them into the main system bus.
Affects machine leon3_generic.  Visible in "info qtree":

     bus: main-system-bus
       type System
    +  dev: grlib,ahbpnp, id ""
    +    mmio 00000000fffff000/0000000000001000
    +  dev: grlib,apbpnp, id ""
    +    mmio 00000000800ff000/0000000000001000
       dev: grlib,irqmp, id ""

Cc: Fabien Chouteau <chouteau@adacore.com>
Cc: KONRAD Frederic <frederic.konrad@adacore.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sparc/leon3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 8f024dab7b..3facb8c2ae 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -213,14 +213,14 @@ static void leon3_generic_hw_init(MachineState *machine)
     reset_info->sp    = LEON3_RAM_OFFSET + ram_size;
     qemu_register_reset(main_cpu_reset, reset_info);
 
-    ahb_pnp = GRLIB_AHB_PNP(object_new(TYPE_GRLIB_AHB_PNP));
+    ahb_pnp = GRLIB_AHB_PNP(qdev_create(NULL, TYPE_GRLIB_AHB_PNP));
     object_property_set_bool(OBJECT(ahb_pnp), true, "realized", &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(ahb_pnp), 0, LEON3_AHB_PNP_OFFSET);
     grlib_ahb_pnp_add_entry(ahb_pnp, 0, 0, GRLIB_VENDOR_GAISLER,
                             GRLIB_LEON3_DEV, GRLIB_AHB_MASTER,
                             GRLIB_CPU_AREA);
 
-    apb_pnp = GRLIB_APB_PNP(object_new(TYPE_GRLIB_APB_PNP));
+    apb_pnp = GRLIB_APB_PNP(qdev_create(NULL, TYPE_GRLIB_APB_PNP));
     object_property_set_bool(OBJECT(apb_pnp), true, "realized", &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(apb_pnp), 0, LEON3_APB_PNP_OFFSET);
     grlib_ahb_pnp_add_entry(ahb_pnp, LEON3_APB_PNP_OFFSET, 0xFFF,
-- 
2.21.1

Re: [PATCH 21/24] sparc/leon3: Fix to put grlib,* devices on sysbus

[Fred Konrad]

Le 5/18/20 à 7:04 AM, Markus Armbruster a écrit :
> leon3_generic_hw_init() creates a "grlib,ahbpnp" and a "grlib,apbpnp"
> sysbus device in a way that leaves them unplugged.
> 
> Create them the common way that puts them into the main system bus.
> Affects machine leon3_generic.  Visible in "info qtree":
> 
>       bus: main-system-bus
>         type System
>      +  dev: grlib,ahbpnp, id ""
>      +    mmio 00000000fffff000/0000000000001000
>      +  dev: grlib,apbpnp, id ""
>      +    mmio 00000000800ff000/0000000000001000
>         dev: grlib,irqmp, id ""
> 
> Cc: Fabien Chouteau <chouteau@adacore.com>
> Cc: KONRAD Frederic <frederic.konrad@adacore.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/sparc/leon3.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 8f024dab7b..3facb8c2ae 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -213,14 +213,14 @@ static void leon3_generic_hw_init(MachineState *machine)
>       reset_info->sp    = LEON3_RAM_OFFSET + ram_size;
>       qemu_register_reset(main_cpu_reset, reset_info);
>   
> -    ahb_pnp = GRLIB_AHB_PNP(object_new(TYPE_GRLIB_AHB_PNP));
> +    ahb_pnp = GRLIB_AHB_PNP(qdev_create(NULL, TYPE_GRLIB_AHB_PNP));
>       object_property_set_bool(OBJECT(ahb_pnp), true, "realized", &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(ahb_pnp), 0, LEON3_AHB_PNP_OFFSET);
>       grlib_ahb_pnp_add_entry(ahb_pnp, 0, 0, GRLIB_VENDOR_GAISLER,
>                               GRLIB_LEON3_DEV, GRLIB_AHB_MASTER,
>                               GRLIB_CPU_AREA);
>   
> -    apb_pnp = GRLIB_APB_PNP(object_new(TYPE_GRLIB_APB_PNP));
> +    apb_pnp = GRLIB_APB_PNP(qdev_create(NULL, TYPE_GRLIB_APB_PNP));
>       object_property_set_bool(OBJECT(apb_pnp), true, "realized", &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(apb_pnp), 0, LEON3_APB_PNP_OFFSET);
>       grlib_ahb_pnp_add_entry(ahb_pnp, LEON3_APB_PNP_OFFSET, 0xFFF,
> 

Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>

Re: [PATCH 21/24] sparc/leon3: Fix to put grlib,* devices on sysbus

[Philippe Mathieu-Daudé]

On 5/18/20 7:04 AM, Markus Armbruster wrote:
> leon3_generic_hw_init() creates a "grlib,ahbpnp" and a "grlib,apbpnp"
> sysbus device in a way that leaves them unplugged.
> 
> Create them the common way that puts them into the main system bus.
> Affects machine leon3_generic.  Visible in "info qtree":
> 
>       bus: main-system-bus
>         type System
>      +  dev: grlib,ahbpnp, id ""
>      +    mmio 00000000fffff000/0000000000001000
>      +  dev: grlib,apbpnp, id ""
>      +    mmio 00000000800ff000/0000000000001000
>         dev: grlib,irqmp, id ""
> 
> Cc: Fabien Chouteau <chouteau@adacore.com>
> Cc: KONRAD Frederic <frederic.konrad@adacore.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/sparc/leon3.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 8f024dab7b..3facb8c2ae 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -213,14 +213,14 @@ static void leon3_generic_hw_init(MachineState *machine)
>       reset_info->sp    = LEON3_RAM_OFFSET + ram_size;
>       qemu_register_reset(main_cpu_reset, reset_info);
>   
> -    ahb_pnp = GRLIB_AHB_PNP(object_new(TYPE_GRLIB_AHB_PNP));
> +    ahb_pnp = GRLIB_AHB_PNP(qdev_create(NULL, TYPE_GRLIB_AHB_PNP));
>       object_property_set_bool(OBJECT(ahb_pnp), true, "realized", &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(ahb_pnp), 0, LEON3_AHB_PNP_OFFSET);
>       grlib_ahb_pnp_add_entry(ahb_pnp, 0, 0, GRLIB_VENDOR_GAISLER,
>                               GRLIB_LEON3_DEV, GRLIB_AHB_MASTER,
>                               GRLIB_CPU_AREA);
>   
> -    apb_pnp = GRLIB_APB_PNP(object_new(TYPE_GRLIB_APB_PNP));
> +    apb_pnp = GRLIB_APB_PNP(qdev_create(NULL, TYPE_GRLIB_APB_PNP));
>       object_property_set_bool(OBJECT(apb_pnp), true, "realized", &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(apb_pnp), 0, LEON3_APB_PNP_OFFSET);
>       grlib_ahb_pnp_add_entry(ahb_pnp, LEON3_APB_PNP_OFFSET, 0xFFF,
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 22/24] qdev: Assert devices are plugged into a bus that can take them

[Markus Armbruster]

This would have caught some of the bugs I just fixed.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/qdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9e5538aeae..0df995eb94 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -97,6 +97,11 @@ static void bus_add_child(BusState *bus, DeviceState *child)
 void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
 {
     BusState *old_parent_bus = dev->parent_bus;
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+
+    assert(dc->bus_type
+           ? bus && object_dynamic_cast(OBJECT(bus), dc->bus_type)
+           : !bus);
 
     if (old_parent_bus) {
         trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),
-- 
2.21.1

Re: [PATCH 22/24] qdev: Assert devices are plugged into a bus that can take them

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> This would have caught some of the bugs I just fixed.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/core/qdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 9e5538aeae..0df995eb94 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -97,6 +97,11 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>  void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>  {
>      BusState *old_parent_bus = dev->parent_bus;
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +
> +    assert(dc->bus_type
> +           ? bus && object_dynamic_cast(OBJECT(bus), dc->bus_type)
> +           : !bus);
>  
>      if (old_parent_bus) {
>          trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),

Actually, !bus crashes below in bus_add_child().  Simpler assertion:

       assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type));

Re: [PATCH 22/24] qdev: Assert devices are plugged into a bus that can take them

[Mark Cave-Ayland]

On 18/05/2020 06:04, Markus Armbruster wrote:

> This would have caught some of the bugs I just fixed.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/core/qdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 9e5538aeae..0df995eb94 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -97,6 +97,11 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>  void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>  {
>      BusState *old_parent_bus = dev->parent_bus;
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +
> +    assert(dc->bus_type
> +           ? bus && object_dynamic_cast(OBJECT(bus), dc->bus_type)
> +           : !bus);
>  
>      if (old_parent_bus) {
>          trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),

Works for me. If you've managed to fix up a large number of bad cases, let's not
allow people to go on making the same mistakes.

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 23/24] sd: Hide the qdev-but-not-quite thing created by sd_init()

[Markus Armbruster]

Commit 260bc9d8aa "hw/sd/sd.c: QOMify" QOMified only the device
itself, not its users.  It kept sd_init() around for non-QOMified
users.

More than four years later, three such users remain: omap1 (machines
cheetah, sx1, sx1-v1) and omap2 (machines n800, n810) are not
QOMified, and pl181 (machines integratorcp, realview-eb,
realview-eb-mpcore, realview-pb-a8 realview-pbx-a9, versatileab,
versatilepb, vexpress-a15, vexpress-a9) is not QOMified properly.

The issue I presently have with this: an "sd-card" device should plug
into an "sd-bus" (its DeviceClass member bus_type says so), but
sd_init() leaves it unplugged.  This is normally a bug (I just fixed
some instances), and I'd like to assert proper pluggedness to prevent
regressions.  However, the qdev-but-not-quite thing returned by
sd_init() would fail the assertion.  Meh.

Make sd_init() hide it from QOM/qdev.  Visible in "info qom-tree",
here's the change for cheetah:

     /machine (cheetah-machine)
       [...]
       /unattached (container)
         [...]
         /device[5] (serial-mm)
           /serial (serial)
           /serial[0] (qemu:memory-region)
    -    /device[6] (sd-card)
    -    /device[7] (omap-gpio)
    +    /device[6] (omap-gpio)
         [rest of device[*] renumbered...]

Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sd/sd.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 71a9af09ab..d7d8b82dfd 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -83,6 +83,10 @@ enum SDCardStates {
 struct SDState {
     DeviceState parent_obj;
 
+    /* If true, created by sd_init() for a non-qdevified caller */
+    /* TODO purge them with fire */
+    bool me_no_qdev_me_kill_mammoth_with_rocks;
+
     /* SD Memory Card Registers */
     uint32_t ocr;
     uint8_t scr[8];
@@ -129,6 +133,8 @@ struct SDState {
     bool cmd_line;
 };
 
+static void sd_realize(DeviceState *dev, Error **errp);
+
 static const char *sd_state_name(enum SDCardStates state)
 {
     static const char *state_name[] = {
@@ -590,7 +596,7 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
 {
     SDState *sd = opaque;
     DeviceState *dev = DEVICE(sd);
-    SDBus *sdbus = SD_BUS(qdev_get_parent_bus(dev));
+    SDBus *sdbus;
     bool inserted = sd_get_inserted(sd);
     bool readonly = sd_get_readonly(sd);
 
@@ -601,19 +607,17 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
         trace_sdcard_ejected();
     }
 
-    /* The IRQ notification is for legacy non-QOM SD controller devices;
-     * QOMified controllers use the SDBus APIs.
-     */
-    if (sdbus) {
-        sdbus_set_inserted(sdbus, inserted);
-        if (inserted) {
-            sdbus_set_readonly(sdbus, readonly);
-        }
-    } else {
+    if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
         qemu_set_irq(sd->inserted_cb, inserted);
         if (inserted) {
             qemu_set_irq(sd->readonly_cb, readonly);
         }
+    } else {
+        sdbus = SD_BUS(qdev_get_parent_bus(dev));
+        sdbus_set_inserted(sdbus, inserted);
+        if (inserted) {
+            sdbus_set_readonly(sdbus, readonly);
+        }
     }
 }
 
@@ -697,6 +701,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
 {
     Object *obj;
     DeviceState *dev;
+    SDState *sd;
     Error *err = NULL;
 
     obj = object_new(TYPE_SD_CARD);
@@ -707,13 +712,24 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
         return NULL;
     }
     qdev_prop_set_bit(dev, "spi", is_spi);
-    object_property_set_bool(obj, true, "realized", &err);
+
+    /*
+     * Realizing the device properly would put it into the QOM
+     * composition tree even though it is not plugged into an
+     * appropriate bus.  That's a no-no.  Hide the device from
+     * QOM/qdev, and call its qdev realize callback directly.
+     */
+    object_ref(obj);
+    object_unparent(obj);
+    sd_realize(dev, &err);
     if (err) {
         error_report("sd_init failed: %s", error_get_pretty(err));
         return NULL;
     }
 
-    return SD_CARD(dev);
+    sd = SD_CARD(dev);
+    sd->me_no_qdev_me_kill_mammoth_with_rocks = true;
+    return sd;
 }
 
 void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
-- 
2.21.1

[PATCH 24/24] qdev: Assert onboard devices all get realized properly

[Markus Armbruster]

This would have caught some of the bugs I just fixed.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/qdev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 0df995eb94..fe2dea8968 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -429,6 +429,19 @@ void qdev_init_nofail(DeviceState *dev)
     object_unref(OBJECT(dev));
 }
 
+static int qdev_assert_realized_properly(Object *obj, void *opaque)
+{
+    DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
+    DeviceClass *dc;
+
+    if (dev) {
+        dc = DEVICE_GET_CLASS(dev);
+        assert(dev->realized);
+        assert(dev->parent_bus || !dc->bus_type);
+    }
+    return 0;
+}
+
 void qdev_machine_creation_done(void)
 {
     /*
@@ -436,6 +449,9 @@ void qdev_machine_creation_done(void)
      * only create hotpluggable devices
      */
     qdev_hotplug = true;
+
+    object_child_foreach_recursive(object_get_root(),
+                                   qdev_assert_realized_properly, NULL);
 }
 
 bool qdev_machine_modified(void)
-- 
2.21.1

Re: [PATCH 24/24] qdev: Assert onboard devices all get realized properly

[Philippe Mathieu-Daudé]

On 5/18/20 7:04 AM, Markus Armbruster wrote:
> This would have caught some of the bugs I just fixed.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/core/qdev.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 0df995eb94..fe2dea8968 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -429,6 +429,19 @@ void qdev_init_nofail(DeviceState *dev)
>       object_unref(OBJECT(dev));
>   }
>   
> +static int qdev_assert_realized_properly(Object *obj, void *opaque)
> +{
> +    DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
> +    DeviceClass *dc;
> +
> +    if (dev) {
> +        dc = DEVICE_GET_CLASS(dev);
> +        assert(dev->realized);
> +        assert(dev->parent_bus || !dc->bus_type);

Nice :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +    }
> +    return 0;
> +}
> +
>   void qdev_machine_creation_done(void)
>   {
>       /*
> @@ -436,6 +449,9 @@ void qdev_machine_creation_done(void)
>        * only create hotpluggable devices
>        */
>       qdev_hotplug = true;
> +
> +    object_child_foreach_recursive(object_get_root(),
> +                                   qdev_assert_realized_properly, NULL);
>   }
>   
>   bool qdev_machine_modified(void)
>