[PATCH v2 00/24] Fixes around device realization

[PATCH v2 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 posted these hacks separately as '[PATCH not-for-merge 0/5]
Instrumentation for "Fixes around device realization"'.  PATCH 2/5
since became "[PATCH 0/2] qom: Make "info qom-tree" show children
sorted".  It should be applied first.

v2:
* Rebased
* PATCH 01: Also fix MMIO addresses, with Alistair's help
* PATCH 04+05: Replaced by better patches from Cédric
* PATCH 01-03+06+08-11+18: Commit messages improved [Peter, Paolo]
* PATCH 08+09+18: Avoid qdev_init_nofail() [Peter]
* PATCH 22: Assertion simplified

Based-on: Message-Id: <20200527084754.7531-1-armbru@redhat.com>

Cédric Le Goater (2):
  arm/aspeed: Compute the number of CPUs from the SoC definition
  arm/aspeed: Rework NIC attachment

Markus Armbruster (22):
  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
  armv7m: Delete unused "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: Delete unused "macio-gpio" devices
  pnv/phb4: Delete unused "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/arm/aspeed.h     |  6 ++++++
 include/hw/arm/aspeed_soc.h |  1 -
 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.c             | 42 ++++++++++++++++++++++++++++++++-----
 hw/arm/aspeed_ast2600.c     | 23 +++++++-------------
 hw/arm/aspeed_soc.c         | 12 ++---------
 hw/arm/stm32f405_soc.c      | 23 +++++++++++---------
 hw/core/qdev.c              | 19 +++++++++++++++++
 hw/display/ati.c            |  2 ++
 hw/display/sm501.c          |  2 ++
 hw/display/xlnx_dp.c        |  4 ++++
 hw/misc/auxbus.c            |  2 +-
 hw/misc/mac_via.c           |  5 +++++
 hw/misc/macio/cuda.c        | 10 ++++-----
 hw/misc/macio/macio.c       |  7 +++++--
 hw/misc/macio/pmu.c         | 10 ++++-----
 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 ++
 31 files changed, 177 insertions(+), 98 deletions(-)

-- 
2.21.3

[PATCH v2 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.

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.

The five unrealized devices appear to stay unreal: neither MMIO nor
IRQ get wired up.

Fix stm32f405_soc_realize() to realize and wire up 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 0000000040012100/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012200/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012300/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio 0000000040012000/00000000000000ff
    +    mmio 0000000040012400/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012500/00000000000000ff
       dev: armv7m, id ""

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 | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
index 4f10ce6176..c9a530eecf 100644
--- a/hw/arm/stm32f405_soc.c
+++ b/hw/arm/stm32f405_soc.c
@@ -37,7 +37,8 @@ static const uint32_t usart_addr[] = { 0x40011000, 0x40004400, 0x40004800,
 /* At the moment only Timer 2 to 5 are modelled */
 static const uint32_t timer_addr[] = { 0x40000000, 0x40000400,
                                        0x40000800, 0x40000C00 };
-#define ADC_ADDR                       0x40012000
+static const uint32_t adc_addr[] = { 0x40012000, 0x40012100, 0x40012200,
+                                     0x40012300, 0x40012400, 0x40012500 };
 static const uint32_t spi_addr[] =   { 0x40013000, 0x40003800, 0x40003C00,
                                        0x40013400, 0x40015000, 0x40015400 };
 #define EXTI_ADDR                      0x40013C00
@@ -185,16 +186,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[i]);
+        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.3

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

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, Markus Armbruster wrote:
> stm32f405_soc_initfn() creates six such devices, but
> stm32f405_soc_realize() realizes only one.  Affects machine
> netduinoplus2.
> 
> 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.
> 
> The five unrealized devices appear to stay unreal: neither MMIO nor
> IRQ get wired up.
> 
> Fix stm32f405_soc_realize() to realize and wire up 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 0000000040012100/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012200/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012300/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio 0000000040012000/00000000000000ff
>     +    mmio 0000000040012400/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012500/00000000000000ff
>        dev: armv7m, id ""
> 
> 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 | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
> index 4f10ce6176..c9a530eecf 100644
> --- a/hw/arm/stm32f405_soc.c
> +++ b/hw/arm/stm32f405_soc.c
> @@ -37,7 +37,8 @@ static const uint32_t usart_addr[] = { 0x40011000, 0x40004400, 0x40004800,
>  /* At the moment only Timer 2 to 5 are modelled */
>  static const uint32_t timer_addr[] = { 0x40000000, 0x40000400,
>                                         0x40000800, 0x40000C00 };
> -#define ADC_ADDR                       0x40012000
> +static const uint32_t adc_addr[] = { 0x40012000, 0x40012100, 0x40012200,
> +                                     0x40012300, 0x40012400, 0x40012500 };
>  static const uint32_t spi_addr[] =   { 0x40013000, 0x40003800, 0x40003C00,
>                                         0x40013400, 0x40015000, 0x40015400 };
>  #define EXTI_ADDR                      0x40013C00
> @@ -185,16 +186,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++) {

Correct fix.

Problem will come back when we'll want to implement a STM SoC with 8
ADCs, modifying the definition... We'll need to remember to unref() again.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +        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[i]);
> +        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++) {
> 

[PATCH v2 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.

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.

These two appear to work.  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 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: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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.3

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

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, Markus Armbruster wrote:
> xlnx_dp_init() creates these two devices, but they're never realized.
> Affects machine xlnx-zcu102.
> 
> 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.
> 
> These two appear to work.  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 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: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.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));

Eh??? Why not realize the bridge in aux_init_bus()?

> +
>      qdev_init_nofail(DEVICE(s->dpcd));
>      aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>  
> +    qdev_init_nofail(DEVICE(s->edid));

This one is OK.

> +
>      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 v2 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 5/28/20 1:04 PM, Markus Armbruster wrote:
>> xlnx_dp_init() creates these two devices, but they're never realized.
>> Affects machine xlnx-zcu102.
>> 
>> 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.
>> 
>> These two appear to work.  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 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: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.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));
>
> Eh??? Why not realize the bridge in aux_init_bus()?

Because then aux_init_bus() is no longer "init", but "init and realize".
Instead: "[PATCH v2 32/58] auxbus: New aux_bus_realize(), pairing with
aux_bus_init()".  Okay?

>> +
>>      qdev_init_nofail(DEVICE(s->dpcd));
>>      aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>>  
>> +    qdev_init_nofail(DEVICE(s->edid));
>
> This one is OK.
>
>> +
>>      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 v2 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Philippe Mathieu-Daudé]

On 6/9/20 7:38 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> On 5/28/20 1:04 PM, Markus Armbruster wrote:
>>> xlnx_dp_init() creates these two devices, but they're never realized.
>>> Affects machine xlnx-zcu102.
>>>
>>> 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.
>>>
>>> These two appear to work.  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 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: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.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));
>>
>> Eh??? Why not realize the bridge in aux_init_bus()?
> 
> Because then aux_init_bus() is no longer "init", but "init and realize".
> Instead: "[PATCH v2 32/58] auxbus: New aux_bus_realize(), pairing with
> aux_bus_init()".  Okay?

It would be easier to follow having aux_bus_realize() introduced first,
then used it directly here. Anyway it is reviewed so I don't have a
problem if this patch goes as it. Thanks.

> 
>>> +
>>>      qdev_init_nofail(DEVICE(s->dpcd));
>>>      aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
>>>  
>>> +    qdev_init_nofail(DEVICE(s->edid));
>>
>> This one is OK.
>>
>>> +
>>>      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 v2 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 6/9/20 7:38 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> 
>>> On 5/28/20 1:04 PM, Markus Armbruster wrote:
>>>> xlnx_dp_init() creates these two devices, but they're never realized.
>>>> Affects machine xlnx-zcu102.
>>>>
>>>> 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.
>>>>
>>>> These two appear to work.  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 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: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.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));
>>>
>>> Eh??? Why not realize the bridge in aux_init_bus()?
>> 
>> Because then aux_init_bus() is no longer "init", but "init and realize".
>> Instead: "[PATCH v2 32/58] auxbus: New aux_bus_realize(), pairing with
>> aux_bus_init()".  Okay?
>
> It would be easier to follow having aux_bus_realize() introduced first,

Point taken.

> then used it directly here. Anyway it is reviewed so I don't have a
> problem if this patch goes as it. Thanks.

Appreciated!

[...]

[PATCH v2 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.

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.

This one appears to work.  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 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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 f9c50ddda5..c32df1b8f9 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.3

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

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, Markus Armbruster 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.
> 
> 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.
> 
> This one appears to work.  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 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>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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 f9c50ddda5..c32df1b8f9 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);
> 

[PATCH v2 04/24] arm/aspeed: Compute the number of CPUs from the SoC definition

[Markus Armbruster]

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

Commit ece09beec457 ("aspeed: introduce a configurable number of CPU
per machine") was a convient change during bringup but the Aspeed SoCs
have a fixed number of CPUs : one for the AST2400 and AST2500, and two
for the AST2600.

When the number of CPUs configured with -smp is less than the SoC's
fixed number, the "unconfigured" CPUs are left unrealized. This can
happen for machines ast2600-evb and tacoma-bmc, where the SoC's fixed
number is 2. To get virtual hardware that matches the physical
hardware, you have to pass -smp cpus=2 (or its sugared form -smp 2).

We normally reject -smp cpus=N when N exceeds the machine's limit.
Except we ignore cpus=2 (and only cpus=2) with a warning for machines
ast2500-evb, palmetto-bmc, romulus-bmc, sonorapass-bmc, swift-bmc, and
witherspoon-bmc.

Remove the "num-cpu" property from the SoC state and use the fixed
number of CPUs defined in the SoC class instead. Compute the default,
min, max number of CPUs of the machine directly from the SoC class
definition.

Machines ast2600-evb and tacoma-bmc now always get their second CPU as
they should. Visible in "info qom-tree"; here's the change for
ast2600-evb:

     /machine (ast2600-evb-machine)
       /peripheral (container)
       /peripheral-anon (container)
       /soc (ast2600-a1)
         /a7mpcore (a15mpcore_priv)
           /a15mp-priv-container[0] (qemu:memory-region)
           /gic (arm_gic)
             /gic_cpu[0] (qemu:memory-region)
             /gic_cpu[1] (qemu:memory-region)
    +        /gic_cpu[2] (qemu:memory-region)
             /gic_dist[0] (qemu:memory-region)
             /gic_vcpu[0] (qemu:memory-region)
             /gic_viface[0] (qemu:memory-region)
             /gic_viface[1] (qemu:memory-region)
    +        /gic_viface[2] (qemu:memory-region)
             /unnamed-gpio-in[0] (irq)
             [...]
    +        /unnamed-gpio-in[160] (irq)
             [same for 161 to 190...]
    +        /unnamed-gpio-in[191] (irq)

Also visible in "info qtree"; here's the change for ast2600-evb:

     bus: main-system-bus
       type System
       dev: a15mpcore_priv, id ""
         gpio-in "" 128
    -    gpio-out "sysbus-irq" 5
    -    num-cpu = 1 (0x1)
    +    gpio-out "sysbus-irq" 10
    +    num-cpu = 2 (0x2)
         num-irq = 160 (0xa0)
         mmio 0000000040460000/0000000000008000
       dev: arm_gic, id ""
    -    gpio-in "" 160
    -    num-cpu = 1 (0x1)
    +    gpio-in "" 192
    +    num-cpu = 2 (0x2)
         num-irq = 160 (0xa0)
         revision = 2 (0x2)
         has-security-extensions = true
         has-virtualization-extensions = true
         num-priority-bits = 8 (0x8)
         mmio ffffffffffffffff/0000000000001000
         mmio ffffffffffffffff/0000000000002000
         mmio ffffffffffffffff/0000000000001000
         mmio ffffffffffffffff/0000000000002000
         mmio ffffffffffffffff/0000000000000100
    +    mmio ffffffffffffffff/0000000000000100
    +    mmio ffffffffffffffff/0000000000000200
         mmio ffffffffffffffff/0000000000000200

The other machines now reject -smp cpus=2 just like -smp cpus=3 and up.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20200519091631.1006073-1-clg@kaod.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Commit message expanded]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/arm/aspeed_soc.h |  1 -
 hw/arm/aspeed.c             | 29 ++++++++++++++++++++++++-----
 hw/arm/aspeed_ast2600.c     | 20 +++++++-------------
 hw/arm/aspeed_soc.c         |  9 +--------
 4 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 78b9f6ae53..914115f3ef 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -40,7 +40,6 @@ typedef struct AspeedSoCState {
 
     /*< public >*/
     ARMCPU cpu[ASPEED_CPUS_NUM];
-    uint32_t num_cpus;
     A15MPPrivState     a7mpcore;
     MemoryRegion *dram_mr;
     MemoryRegion sram;
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 2c23297edf..fd5cc542a5 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -283,8 +283,6 @@ 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), machine->smp.cpus, "num-cpus",
-                            &error_abort);
     object_property_set_link(OBJECT(&bmc->soc), OBJECT(&bmc->ram_container),
                              "dram", &error_abort);
     if (machine->kernel_filename) {
@@ -337,7 +335,7 @@ static void aspeed_machine_init(MachineState *machine)
         }
     }
 
-    if (machine->kernel_filename && bmc->soc.num_cpus > 1) {
+    if (machine->kernel_filename && sc->num_cpus > 1) {
         /* With no u-boot we must set up a boot stub for the secondary CPU */
         MemoryRegion *smpboot = g_new(MemoryRegion, 1);
         memory_region_init_ram(smpboot, OBJECT(bmc), "aspeed.smpboot",
@@ -352,7 +350,7 @@ static void aspeed_machine_init(MachineState *machine)
 
     aspeed_board_binfo.ram_size = ram_size;
     aspeed_board_binfo.loader_start = sc->memmap[ASPEED_SDRAM];
-    aspeed_board_binfo.nb_cpus = bmc->soc.num_cpus;
+    aspeed_board_binfo.nb_cpus = sc->num_cpus;
 
     if (amc->i2c_init) {
         amc->i2c_init(bmc);
@@ -549,12 +547,17 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
                            "boot directly from CE0 flash device");
 }
 
+static int aspeed_soc_num_cpus(const char *soc_name)
+{
+   AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(soc_name));
+   return sc->num_cpus;
+}
+
 static void aspeed_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
 
     mc->init = aspeed_machine_init;
-    mc->max_cpus = ASPEED_CPUS_NUM;
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
@@ -576,6 +579,8 @@ static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 1;
     amc->i2c_init  = palmetto_bmc_i2c_init;
     mc->default_ram_size       = 256 * MiB;
+    mc->default_cpus = mc->min_cpus = mc->max_cpus =
+        aspeed_soc_num_cpus(amc->soc_name);
 };
 
 static void aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)
@@ -591,6 +596,8 @@ static void aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 1;
     amc->i2c_init  = ast2500_evb_i2c_init;
     mc->default_ram_size       = 512 * MiB;
+    mc->default_cpus = mc->min_cpus = mc->max_cpus =
+        aspeed_soc_num_cpus(amc->soc_name);
 };
 
 static void aspeed_machine_romulus_class_init(ObjectClass *oc, void *data)
@@ -606,6 +613,8 @@ static void aspeed_machine_romulus_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 2;
     amc->i2c_init  = romulus_bmc_i2c_init;
     mc->default_ram_size       = 512 * MiB;
+    mc->default_cpus = mc->min_cpus = mc->max_cpus =
+        aspeed_soc_num_cpus(amc->soc_name);
 };
 
 static void aspeed_machine_sonorapass_class_init(ObjectClass *oc, void *data)
@@ -621,6 +630,8 @@ static void aspeed_machine_sonorapass_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 2;
     amc->i2c_init  = sonorapass_bmc_i2c_init;
     mc->default_ram_size       = 512 * MiB;
+    mc->default_cpus = mc->min_cpus = mc->max_cpus =
+        aspeed_soc_num_cpus(amc->soc_name);
 };
 
 static void aspeed_machine_swift_class_init(ObjectClass *oc, void *data)
@@ -636,6 +647,8 @@ static void aspeed_machine_swift_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 2;
     amc->i2c_init  = swift_bmc_i2c_init;
     mc->default_ram_size       = 512 * MiB;
+    mc->default_cpus = mc->min_cpus = mc->max_cpus =
+        aspeed_soc_num_cpus(amc->soc_name);
 };
 
 static void aspeed_machine_witherspoon_class_init(ObjectClass *oc, void *data)
@@ -651,6 +664,8 @@ static void aspeed_machine_witherspoon_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 2;
     amc->i2c_init  = witherspoon_bmc_i2c_init;
     mc->default_ram_size = 512 * MiB;
+    mc->default_cpus = mc->min_cpus = mc->max_cpus =
+        aspeed_soc_num_cpus(amc->soc_name);
 };
 
 static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
@@ -667,6 +682,8 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 1;
     amc->i2c_init  = ast2600_evb_i2c_init;
     mc->default_ram_size = 1 * GiB;
+    mc->default_cpus = mc->min_cpus = mc->max_cpus =
+        aspeed_soc_num_cpus(amc->soc_name);
 };
 
 static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
@@ -683,6 +700,8 @@ static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 2;
     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 =
+        aspeed_soc_num_cpus(amc->soc_name);
 };
 
 static const TypeInfo aspeed_machine_types[] = {
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 71a0acfe26..c6821b3322 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -255,17 +255,11 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     create_unimplemented_device("aspeed.video", sc->memmap[ASPEED_VIDEO],
                                 0x1000);
 
-    if (s->num_cpus > sc->num_cpus) {
-        warn_report("%s: invalid number of CPUs %d, using default %d",
-                    sc->name, s->num_cpus, sc->num_cpus);
-        s->num_cpus = sc->num_cpus;
-    }
-
     /* CPU */
-    for (i = 0; i < s->num_cpus; i++) {
+    for (i = 0; i < sc->num_cpus; i++) {
         object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
                                 "psci-conduit", &error_abort);
-        if (s->num_cpus > 1) {
+        if (sc->num_cpus > 1) {
             object_property_set_int(OBJECT(&s->cpu[i]),
                                     ASPEED_A7MPCORE_ADDR,
                                     "reset-cbar", &error_abort);
@@ -289,7 +283,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     }
 
     /* A7MPCORE */
-    object_property_set_int(OBJECT(&s->a7mpcore), s->num_cpus, "num-cpu",
+    object_property_set_int(OBJECT(&s->a7mpcore), sc->num_cpus, "num-cpu",
                             &error_abort);
     object_property_set_int(OBJECT(&s->a7mpcore),
                             ASPEED_SOC_AST2600_MAX_IRQ + GIC_INTERNAL,
@@ -299,18 +293,18 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
                              &error_abort);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, ASPEED_A7MPCORE_ADDR);
 
-    for (i = 0; i < s->num_cpus; i++) {
+    for (i = 0; i < sc->num_cpus; i++) {
         SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
         DeviceState  *d   = DEVICE(qemu_get_cpu(i));
 
         irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
         sysbus_connect_irq(sbd, i, irq);
         irq = qdev_get_gpio_in(d, ARM_CPU_FIQ);
-        sysbus_connect_irq(sbd, i + s->num_cpus, irq);
+        sysbus_connect_irq(sbd, i + sc->num_cpus, irq);
         irq = qdev_get_gpio_in(d, ARM_CPU_VIRQ);
-        sysbus_connect_irq(sbd, i + 2 * s->num_cpus, irq);
+        sysbus_connect_irq(sbd, i + 2 * sc->num_cpus, irq);
         irq = qdev_get_gpio_in(d, ARM_CPU_VFIQ);
-        sysbus_connect_irq(sbd, i + 3 * s->num_cpus, irq);
+        sysbus_connect_irq(sbd, i + 3 * sc->num_cpus, irq);
     }
 
     /* SRAM */
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index cf6b6dd116..e6f4b59134 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -242,14 +242,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     create_unimplemented_device("aspeed.video", sc->memmap[ASPEED_VIDEO],
                                 0x1000);
 
-    if (s->num_cpus > sc->num_cpus) {
-        warn_report("%s: invalid number of CPUs %d, using default %d",
-                    sc->name, s->num_cpus, sc->num_cpus);
-        s->num_cpus = sc->num_cpus;
-    }
-
     /* CPU */
-    for (i = 0; i < s->num_cpus; i++) {
+    for (i = 0; i < sc->num_cpus; i++) {
         object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
@@ -460,7 +454,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                        aspeed_soc_get_irq(s, ASPEED_SDHCI));
 }
 static Property aspeed_soc_properties[] = {
-    DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
     DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_END_OF_LIST(),
-- 
2.21.3

[PATCH v2 05/24] arm/aspeed: Rework NIC attachment

[Markus Armbruster]

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

The number of MACs supported by an Aspeed SoC is defined by "macs_num"
under the SoC model, that is two for the AST2400 and AST2500 and four
for the AST2600. The model initializes the maximum number of supported
MACs but the number of realized devices is capped by the number of
network device back-ends defined on the command line. This can leave
unrealized devices hanging around in the QOM composition tree.

To get virtual hardware that matches the physical hardware, you have
to pass exactly as many -nic options as there are MACs, and some of
them must be -nic none:

* Machines ast2500-evb, palmetto-bmc, romulus-bmc, sonorapass-bmc,
  swift-bmc, and witherspoon-bmc: two -nic, and the second one must be
  -nic none.

* Machine ast2600-evb: four -nic, the first one must be -nic none.

* Machine tacoma-bmc: four nic, the first two and the last one must be
  -nic none.

Modify the machine initialization to define which MACs are attached to
a network device back-end using a bit-field property "macs-mask" and
let the SoC realize all network devices.

The default setting of "macs-mask" is "use MAC0" only, which works for
all our AST2400 and AST2500 machines. The AST2600 machines have
different configurations. The AST2600 EVB machine activates MAC1, MAC2
and MAC3 and the Tacoma BMC machine activates MAC2.

Incompatible CLI change: -nic options now apply to *active* MACs:
MAC1, MAC2, MAC3 for ast2600-evb, MAC2 for tacoma-bmc, and MAC0 for
all the others.

The machines now always get all MACs as they should. Visible in "info
qom-tree", here's the change for tacoma-bmc:

     /machine (tacoma-bmc-machine)
       /peripheral (container)
       /peripheral-anon (container)
       /soc (ast2600-a1)
         [...]
         /ftgmac100[0] (ftgmac100)
           /ftgmac100[0] (qemu:memory-region)
         /ftgmac100[1] (ftgmac100)
    +      /ftgmac100[0] (qemu:memory-region)
         /ftgmac100[2] (ftgmac100)
    +      /ftgmac100[0] (qemu:memory-region)
         /ftgmac100[3] (ftgmac100)
    +      /ftgmac100[0] (qemu:memory-region)
         [...]
         /mii[0] (aspeed-mmi)
           /aspeed-mmi[0] (qemu:memory-region)
         /mii[1] (aspeed-mmi)
    +      /aspeed-mmi[0] (qemu:memory-region)
         /mii[2] (aspeed-mmi)
    +      /aspeed-mmi[0] (qemu:memory-region)
         /mii[3] (aspeed-mmi)
    +      /aspeed-mmi[0] (qemu:memory-region)

Also visible in "info qtree"; here's the change for tacoma-bmc:

       dev: ftgmac100, id ""
         gpio-out "sysbus-irq" 1
         aspeed = true
    -    mac = "52:54:00:12:34:56"
    -    netdev = "hub0port0"
    +    mac = "52:54:00:12:34:57"
    +    netdev = ""
         mmio 000000001e660000/0000000000002000
       dev: ftgmac100, id ""
    -    aspeed = false
    -    mac = "00:00:00:00:00:00"
    +    gpio-out "sysbus-irq" 1
    +    aspeed = true
    +    mac = "52:54:00:12:34:58"
         netdev = ""
    +    mmio 000000001e680000/0000000000002000
       dev: ftgmac100, id ""
    -    aspeed = false
    -    mac = "00:00:00:00:00:00"
    -    netdev = ""
    +    gpio-out "sysbus-irq" 1
    +    aspeed = true
    +    mac = "52:54:00:12:34:56"
    +    netdev = "hub0port0"
    +    mmio 000000001e670000/0000000000002000
       dev: ftgmac100, id ""
    -    aspeed = false
    -    mac = "00:00:00:00:00:00"
    +    gpio-out "sysbus-irq" 1
    +    aspeed = true
    +    mac = "52:54:00:12:34:59"
         netdev = ""
    +    mmio 000000001e690000/0000000000002000
       [...]
       dev: aspeed-mmi, id ""
         mmio 000000001e650000/0000000000000008
       dev: aspeed-mmi, id ""
    +    mmio 000000001e650008/0000000000000008
       dev: aspeed-mmi, id ""
    +    mmio 000000001e650010/0000000000000008
       dev: aspeed-mmi, id ""
    +    mmio 000000001e650018/0000000000000008

Inactive MACs will have no peer and QEMU may warn the user with :

    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

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200527124406.329503-1-clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
[Commit message expanded]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/arm/aspeed.h |  6 ++++++
 hw/arm/aspeed.c         | 13 +++++++++++++
 hw/arm/aspeed_ast2600.c |  3 +--
 hw/arm/aspeed_soc.c     |  3 +--
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 18521484b9..95b4daece8 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -23,6 +23,11 @@ typedef struct AspeedMachine {
     bool mmio_exec;
 } AspeedMachine;
 
+#define ASPEED_MAC0_ON   (1 << 0)
+#define ASPEED_MAC1_ON   (1 << 1)
+#define ASPEED_MAC2_ON   (1 << 2)
+#define ASPEED_MAC3_ON   (1 << 3)
+
 #define ASPEED_MACHINE_CLASS(klass) \
      OBJECT_CLASS_CHECK(AspeedMachineClass, (klass), TYPE_ASPEED_MACHINE)
 #define ASPEED_MACHINE_GET_CLASS(obj) \
@@ -39,6 +44,7 @@ typedef struct AspeedMachineClass {
     const char *fmc_model;
     const char *spi_model;
     uint32_t num_cs;
+    uint32_t macs_mask;
     void (*i2c_init)(AspeedBoardState *bmc);
 } AspeedMachineClass;
 
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index fd5cc542a5..f8f3ef89d3 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -258,6 +258,7 @@ static void aspeed_machine_init(MachineState *machine)
     DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
     ram_addr_t max_ram_size;
     int i;
+    NICInfo *nd = &nd_table[0];
 
     bmc = g_new0(AspeedBoardState, 1);
 
@@ -277,6 +278,14 @@ static void aspeed_machine_init(MachineState *machine)
     object_property_set_uint(OBJECT(&bmc->soc), ram_size, "ram-size",
                              &error_fatal);
 
+    for (i = 0; i < sc->macs_num; i++) {
+        if ((amc->macs_mask & (1 << i)) && nd->used) {
+            qemu_check_nic_model(nd, TYPE_FTGMAC100);
+            qdev_set_nic_properties(DEVICE(&bmc->soc.ftgmac100[i]), nd);
+            nd++;
+        }
+    }
+
     object_property_set_int(OBJECT(&bmc->soc), amc->hw_strap1, "hw-strap1",
                             &error_abort);
     object_property_set_int(OBJECT(&bmc->soc), amc->hw_strap2, "hw-strap2",
@@ -556,12 +565,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->macs_mask = ASPEED_MAC0_ON;
 
     aspeed_machine_class_props_init(oc);
 }
@@ -680,6 +691,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
     amc->fmc_model = "w25q512jv";
     amc->spi_model = "mx66u51235f";
     amc->num_cs    = 1;
+    amc->macs_mask  = ASPEED_MAC1_ON | ASPEED_MAC2_ON | ASPEED_MAC3_ON;
     amc->i2c_init  = ast2600_evb_i2c_init;
     mc->default_ram_size = 1 * GiB;
     mc->default_cpus = mc->min_cpus = mc->max_cpus =
@@ -698,6 +710,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->macs_mask  = ASPEED_MAC2_ON;
     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 c6821b3322..b912d19f80 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -461,8 +461,7 @@ 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++) {
         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 e6f4b59134..3ec1257c14 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -404,8 +404,7 @@ 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++) {
         object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
                                  &err);
         object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "realized",
-- 
2.21.3

[PATCH v2 06/24] armv7m: Delete unused "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.

Delete the unused 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.3

Re: [PATCH v2 06/24] armv7m: Delete unused "ARM,bitband-memory" devices

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, Markus Armbruster 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.
> 
> Delete the unused 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>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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]));
>          }
>      }
>  }
> 

Re: [PATCH v2 06/24] armv7m: Delete unused "ARM, bitband-memory" devices

[Peter Maydell]

On Thu, 28 May 2020 at 12: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.
>
> Delete the unused 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>

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

thanks
-- PMM

[PATCH v2 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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.3

[PATCH v2 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.

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.

These two appear to work.  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 right away.

Fixes: 6dca62a0000f95e0b7020aa00d0ca9b2c421f341
Cc: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mac_via.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index e05623d730..82614c155a 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -890,6 +890,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
     object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
                               SYSBUS_DEVICE_GPIO_IRQ "[0]");
 
+    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
+                             &error_abort);
+    object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
+                             &error_abort);
+
     /* 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.3

[PATCH v2 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,

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.

These onetwo appear to work.  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 | 10 +++++-----
 hw/misc/macio/pmu.c  | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index e0cc0aac5d..763a785f1a 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -33,6 +33,7 @@
 #include "hw/misc/macio/cuda.h"
 #include "qemu/timer.h"
 #include "sysemu/runstate.h"
+#include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error **errp)
 {
     CUDAState *s = CUDA(dev);
     SysBusDevice *sbd;
-    MOS6522State *ms;
-    DeviceState *d;
     struct tm tm;
 
+    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
+                             &error_abort);
+
     /* 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..4264779396 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -40,6 +40,7 @@
 #include "hw/misc/macio/pmu.h"
 #include "qemu/timer.h"
 #include "sysemu/runstate.h"
+#include "qapi/error.h"
 #include "qemu/cutils.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -740,15 +741,14 @@ static void pmu_realize(DeviceState *dev, Error **errp)
 {
     PMUState *s = VIA_PMU(dev);
     SysBusDevice *sbd;
-    MOS6522State *ms;
-    DeviceState *d;
     struct tm tm;
 
+    object_property_set_bool(OBJECT(&s->mos6522_pmu), true, "realized",
+                             &error_abort);
+
     /* 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.3

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

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, Markus Armbruster 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,
> 
> 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.
> 
> These onetwo appear to work.  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 | 10 +++++-----
>  hw/misc/macio/pmu.c  | 10 +++++-----
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index e0cc0aac5d..763a785f1a 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -33,6 +33,7 @@
>  #include "hw/misc/macio/cuda.h"
>  #include "qemu/timer.h"
>  #include "sysemu/runstate.h"
> +#include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> @@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>  {
>      CUDAState *s = CUDA(dev);
>      SysBusDevice *sbd;
> -    MOS6522State *ms;
> -    DeviceState *d;
>      struct tm tm;
>  
> +    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
> +                             &error_abort);

Either use local_err and return on error, or simpler realize it in
cuda_init()...

> +
>      /* 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..4264779396 100644
> --- a/hw/misc/macio/pmu.c
> +++ b/hw/misc/macio/pmu.c
> @@ -40,6 +40,7 @@
>  #include "hw/misc/macio/pmu.h"
>  #include "qemu/timer.h"
>  #include "sysemu/runstate.h"
> +#include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> @@ -740,15 +741,14 @@ static void pmu_realize(DeviceState *dev, Error **errp)
>  {
>      PMUState *s = VIA_PMU(dev);
>      SysBusDevice *sbd;
> -    MOS6522State *ms;
> -    DeviceState *d;
>      struct tm tm;
>  
> +    object_property_set_bool(OBJECT(&s->mos6522_pmu), true, "realized",
> +                             &error_abort);

Ditto.

> +
>      /* 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;
> 

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

[Peter Maydell]

On Thu, 28 May 2020 at 12:13, 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,
>
> 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.
>
> These onetwo appear to work.  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 | 10 +++++-----
>  hw/misc/macio/pmu.c  | 10 +++++-----
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index e0cc0aac5d..763a785f1a 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -33,6 +33,7 @@
>  #include "hw/misc/macio/cuda.h"
>  #include "qemu/timer.h"
>  #include "sysemu/runstate.h"
> +#include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> @@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>  {
>      CUDAState *s = CUDA(dev);
>      SysBusDevice *sbd;
> -    MOS6522State *ms;
> -    DeviceState *d;
>      struct tm tm;
>
> +    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
> +                             &error_abort);

Still disagree with barfing on failure when we have a perfectly
good way to return the failure indication.

thanks
-- PMM

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

[Markus Armbruster]

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

> On Thu, 28 May 2020 at 12:13, 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,
>>
>> 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.
>>
>> These onetwo appear to work.  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 | 10 +++++-----
>>  hw/misc/macio/pmu.c  | 10 +++++-----
>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index e0cc0aac5d..763a785f1a 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -33,6 +33,7 @@
>>  #include "hw/misc/macio/cuda.h"
>>  #include "qemu/timer.h"
>>  #include "sysemu/runstate.h"
>> +#include "qapi/error.h"
>>  #include "qemu/cutils.h"
>>  #include "qemu/log.h"
>>  #include "qemu/module.h"
>> @@ -523,15 +524,14 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>>  {
>>      CUDAState *s = CUDA(dev);
>>      SysBusDevice *sbd;
>> -    MOS6522State *ms;
>> -    DeviceState *d;
>>      struct tm tm;
>>
>> +    object_property_set_bool(OBJECT(&s->mos6522_cuda), true, "realized",
>> +                             &error_abort);
>
> Still disagree with barfing on failure when we have a perfectly
> good way to return the failure indication.

My patch is a strict improvement: it fixes a bug, and it does not add
ways to fail (the object_property_set_bool() above can't actually fail).

You're asking for additional improvement.  "One may always ask, and one
may always say no."

Since there is nothing to clean up here, I'll stick in the useless error
handling so we can move on.

If the error handling you ask for involved cleanup, I'd say no.

Incorrect unreachable cleanup is worse than &error_abort.  I'm not going
to waste time on unreachable and untestable error handling unless it's
utterly trivial, and I'm certainly not going to waste time on creating
more elaborate time bombs.  I *am* going to waste time managing
expectations, if I have to :)

I feel I have to now, because I feel I've once again stretched my
employer's (awesomely generous!) patience with me doing work that won't
ever go into any of our products to the limit.

[PATCH v2 10/24] macio: Delete unused "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).

Delete the unused 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.3

Re: [PATCH v2 10/24] macio: Delete unused "macio-gpio" devices

[Mark Cave-Ayland]

On 28/05/2020 12:04, 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).
> 
> Delete the unused 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 looks correct to me. Re-reading over the 3 different patch series you've posted,
I think it makes sense to merge them soon since even though there may be some debate
over init/realize in places, the benefits would far outweigh the drawbacks IMO.

Anyhow for this patch:

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


ATB,

Mark.

Re: [PATCH v2 10/24] macio: Delete unused "macio-gpio" devices

[Markus Armbruster]

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

> On 28/05/2020 12:04, 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).
>> 
>> Delete the unused 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 looks correct to me. Re-reading over the 3 different patch series you've posted,
> I think it makes sense to merge them soon since even though there may be some debate
> over init/realize in places, the benefits would far outweigh the drawbacks IMO.

We can always improve on top.

Rebasing this stuff is quite time-consuming.

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

Thanks!

[PATCH v2 11/24] pnv/phb4: Delete unused "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.

Delete the unused 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: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@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,
-- 
2.21.3

[PATCH v2 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>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3690f313c3..3bd5c613f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1213,7 +1213,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.3

[PATCH v2 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Thomas Huth <thuth@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.3

[PATCH v2 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>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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.3

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

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, 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>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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);
> 

[PATCH v2 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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.3

[PATCH v2 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>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 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.3

[PATCH v2 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>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 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.3

[PATCH v2 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.

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.

This one appears to work.  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 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: c82c7336de58876862e6b4dccbda29e9240fd388
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>
Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/ati.c   | 2 ++
 hw/display/sm501.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 065f197678..5c71e5f295 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -929,6 +929,8 @@ 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);
+    object_property_set_bool(OBJECT(i2cddc), true, "realized",
+                             &error_abort);
 
     /* 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..fbedc56715 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1816,6 +1816,8 @@ 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);
+    object_property_set_bool(OBJECT(ddc), true, "realized",
+                             &error_abort);
 
     /* mmio */
     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
-- 
2.21.3

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

[Aleksandar Markovic]

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

Just adding Huacai, the original author and the new maintainer for Fuloong
2E machine.

1:04 PM Čet, 28.05.2020. Markus Armbruster <armbru@redhat.com> је
написао/ла:
>
> sm501_init() and ati_vga_realize() create an "i2c-ddc" device, but
> neglect to realize it.  Affects machines sam460ex, shix, r2d, and
> fulong2e.
>
> 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.
>
> This one appears to work.  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 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: c82c7336de58876862e6b4dccbda29e9240fd388
> 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>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/ati.c   | 2 ++
>  hw/display/sm501.c | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 065f197678..5c71e5f295 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -929,6 +929,8 @@ 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);
> +    object_property_set_bool(OBJECT(i2cddc), true, "realized",
> +                             &error_abort);
>
>      /* 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..fbedc56715 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1816,6 +1816,8 @@ 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);
> +    object_property_set_bool(OBJECT(ddc), true, "realized",
> +                             &error_abort);
>
>      /* mmio */
>      memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio",
MMIO_SIZE);
> --
> 2.21.3
>

[-- Attachment #2: Type: text/html, Size: 4178 bytes --]

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

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, 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.
> 
> 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.
> 
> This one appears to work.  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 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: c82c7336de58876862e6b4dccbda29e9240fd388
> 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>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/display/ati.c   | 2 ++
>  hw/display/sm501.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 065f197678..5c71e5f295 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -929,6 +929,8 @@ 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);
> +    object_property_set_bool(OBJECT(i2cddc), true, "realized",
> +                             &error_abort);
>  
>      /* 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..fbedc56715 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1816,6 +1816,8 @@ 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);
> +    object_property_set_bool(OBJECT(ddc), true, "realized",
> +                             &error_abort);
>  
>      /* mmio */
>      memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
> 

[PATCH v2 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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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 7ce28895bc..0c5bcdc37f 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.3

[PATCH v2 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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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 7ce28895bc..0c5bcdc37f 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.3

[PATCH v2 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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.3

[PATCH v2 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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.3

[PATCH v2 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>
Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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.3

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

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, 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>
> Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@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,
> 

Thanks, patch applied to for the next (temporary) sparc-next pull request.

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

[Philippe Mathieu-Daudé]

On 6/9/20 7:15 AM, Philippe Mathieu-Daudé wrote:
> On 5/28/20 1:04 PM, 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>
>> Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@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,
>>
> 
> Thanks, patch applied to for the next (temporary) sparc-next pull request.

As suggested on IRC, dropping this patch, since it is part of an omnibus
cleanup.

[PATCH v2 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>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/core/qdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9e5538aeae..b5b42b2616 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -97,6 +97,9 @@ 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 && object_dynamic_cast(OBJECT(bus), dc->bus_type));
 
     if (old_parent_bus) {
         trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),
-- 
2.21.3

[PATCH v2 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 3c06a0ac6d..7070a116ea 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_reportf_err(err, "sd_init failed: ");
         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.3

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

[Philippe Mathieu-Daudé]

On 5/28/20 1:04 PM, Markus Armbruster wrote:
> 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 3c06a0ac6d..7070a116ea 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;

Your next patch does not use me_no_qdev_me_kill_mammoth_with_rocks in
qdev_assert_realized_properly().

Suggestion for less ugly hack:

static int qdev_assert_realized_properly(Object *obj, void *opaque)
{
    DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
    DeviceClass *dc;

    if (dev) {
        if (object_dynamic_cast(OBJECT(obj), TYPE_SD_CARD)) {
            /* bla bla bla */
            return 17;
        }
        dc = DEVICE_GET_CLASS(dev);
        assert(dev->realized);
        assert(dev->parent_bus || !dc->bus_type);
    }
    return 0;
}

> +
>      /* 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_reportf_err(err, "sd_init failed: ");
>          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)
> 

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

[Markus Armbruster]

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

> On 5/28/20 1:04 PM, Markus Armbruster wrote:
>> 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 3c06a0ac6d..7070a116ea 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;
>
> Your next patch does not use me_no_qdev_me_kill_mammoth_with_rocks in
> qdev_assert_realized_properly().

It doesn't have to, because this qdev-but-not-quite thing isn't visible
there.

> Suggestion for less ugly hack:
>
> static int qdev_assert_realized_properly(Object *obj, void *opaque)
> {
>     DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
>     DeviceClass *dc;
>
>     if (dev) {
>         if (object_dynamic_cast(OBJECT(obj), TYPE_SD_CARD)) {
>             /* bla bla bla */
>             return 17;
>         }
>         dc = DEVICE_GET_CLASS(dev);
>         assert(dev->realized);
>         assert(dev->parent_bus || !dc->bus_type);
>     }
>     return 0;
> }

Now qdev_assert_realized_properly() knows about the caveman.  I don't
like that.

My hack keeps the knowledge strictly local, and protects all users of
QOM from getting exposed to the caveman, not just the "realized
properly" assertion.  My hack is locally ugly, but I consider that a
feature ;)

My patch could be made smaller: @me_no_qdev_me_kill_mammoth_with_rocks
exists only to make the parts supporting the caveman more immediately
obvious.

>
>> +
>>      /* 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_reportf_err(err, "sd_init failed: ");
>>          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)
>> 

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

[Philippe Mathieu-Daudé]

On 6/9/20 8:39 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 5/28/20 1:04 PM, Markus Armbruster wrote:
>>> 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 3c06a0ac6d..7070a116ea 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;
>>
>> Your next patch does not use me_no_qdev_me_kill_mammoth_with_rocks in
>> qdev_assert_realized_properly().
> 
> It doesn't have to, because this qdev-but-not-quite thing isn't visible
> there.
> 
>> Suggestion for less ugly hack:
>>
>> static int qdev_assert_realized_properly(Object *obj, void *opaque)
>> {
>>     DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
>>     DeviceClass *dc;
>>
>>     if (dev) {
>>         if (object_dynamic_cast(OBJECT(obj), TYPE_SD_CARD)) {
>>             /* bla bla bla */
>>             return 17;
>>         }
>>         dc = DEVICE_GET_CLASS(dev);
>>         assert(dev->realized);
>>         assert(dev->parent_bus || !dc->bus_type);
>>     }
>>     return 0;
>> }
> 
> Now qdev_assert_realized_properly() knows about the caveman.  I don't
> like that.
> 
> My hack keeps the knowledge strictly local, and protects all users of
> QOM from getting exposed to the caveman, not just the "realized
> properly" assertion.  My hack is locally ugly, but I consider that a
> feature ;)

Understood.

> 
> My patch could be made smaller: @me_no_qdev_me_kill_mammoth_with_rocks
> exists only to make the parts supporting the caveman more immediately
> obvious.
> 
>>
>>> +
>>>      /* 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_reportf_err(err, "sd_init failed: ");
>>>          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)
>>>
> 

[PATCH v2 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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 b5b42b2616..a68ba674db 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -427,6 +427,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)
 {
     /*
@@ -434,6 +447,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.3

Re: [PATCH v2 00/24] Fixes around device realization

[Markus Armbruster]

Needs a rebase now.

Paolo, would you like me to post the pull requests for my recent QOM
work myself?

Re: [PATCH v2 00/24] Fixes around device realization

[Paolo Bonzini]

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

Yes please, my next pull request is already at 120 patches...

Paolo

Il ven 5 giu 2020, 17:01 Markus Armbruster <armbru@redhat.com> ha scritto:

> Needs a rebase now.
>
> Paolo, would you like me to post the pull requests for my recent QOM
> work myself?
>
>

[-- Attachment #2: Type: text/html, Size: 580 bytes --]

Re: [PATCH v2 00/24] Fixes around device realization

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> 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 posted these hacks separately as '[PATCH not-for-merge 0/5]
> Instrumentation for "Fixes around device realization"'.  PATCH 2/5
> since became "[PATCH 0/2] qom: Make "info qom-tree" show children
> sorted".  It should be applied first.
>
> v2:
> * Rebased
> * PATCH 01: Also fix MMIO addresses, with Alistair's help
> * PATCH 04+05: Replaced by better patches from Cédric
> * PATCH 01-03+06+08-11+18: Commit messages improved [Peter, Paolo]
> * PATCH 08+09+18: Avoid qdev_init_nofail() [Peter]
> * PATCH 22: Assertion simplified
>
> Based-on: Message-Id: <20200527084754.7531-1-armbru@redhat.com>

Peter, you commented on v1 of PATCH 06 and 09.  Please review v2.

Mark, you commented on v1 of PATCH 10.  Please review v2.

PATCH 18 needs review.  Philippe, perhaps?

Re: [PATCH v2 00/24] Fixes around device realization

[Mark Cave-Ayland]

On 08/06/2020 12:08, Markus Armbruster wrote:

> Markus Armbruster <armbru@redhat.com> writes:
> 
>> 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 posted these hacks separately as '[PATCH not-for-merge 0/5]
>> Instrumentation for "Fixes around device realization"'.  PATCH 2/5
>> since became "[PATCH 0/2] qom: Make "info qom-tree" show children
>> sorted".  It should be applied first.
>>
>> v2:
>> * Rebased
>> * PATCH 01: Also fix MMIO addresses, with Alistair's help
>> * PATCH 04+05: Replaced by better patches from Cédric
>> * PATCH 01-03+06+08-11+18: Commit messages improved [Peter, Paolo]
>> * PATCH 08+09+18: Avoid qdev_init_nofail() [Peter]
>> * PATCH 22: Assertion simplified
>>
>> Based-on: Message-Id: <20200527084754.7531-1-armbru@redhat.com>
> 
> Peter, you commented on v1 of PATCH 06 and 09.  Please review v2.
> 
> Mark, you commented on v1 of PATCH 10.  Please review v2.
> 
> PATCH 18 needs review.  Philippe, perhaps?

Will do. It has been sat on my TODO list for a while, so I'll quickly have a look now.


ATB,

Mark.