Re: [qemu-s390x] [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Philippe Mathieu-Daudé]

On 5/29/19 8:08 AM, Markus Armbruster wrote:
> Peter Maydell <address@hidden> writes:
> 
>> On Mon, 27 May 2019 at 08:52, Markus Armbruster <address@hidden> wrote:
>>>
>>> Peter Maydell <address@hidden> writes:
>>>> Suggestions for how to restructure reset so this doesn't
>>>> happen are welcome... "reset follows the bus hierarchy"
>>>> works well in some places but is a bit weird in others
>>>> (for SoC containers and the like "follow the QOM
>>>> hierarchy" would make more sense, but I have no idea
>>>> how to usefully transition to a model where you could
>>>> say "for these devices, follow QOM tree for reset" or
>>>> what an API for that would look like).
>>>
>>> Here's a QOM composition tree for the ARM virt machine (-nodefaults
>>> -device e1000) as visible in qom-fuse under /machine, with irq and
>>> qemu:memory-region ommitted for brevity:
>>
>> virt is a bit of an outlier because as a purely-virtual
>> machine it has no "SoC" -- it's just a bag of devices
>> at the machine level. It would be interesting to
>> also look at a machine that's emulating something
>> closer to real hardware (eg one of the aspeed machines,
>> or mps2-an521).
> 
> Here you go: witherspoon-bmc (aspeed SoC) with -nodefaults and -device
> m25p80 -device m25p80,id=qdev-id.  The -device are purely for
> illustrating how user-plugged devices get added to the two trees.  I'm
> not claiming they make sense.
> 
> QOM composition tree as visible in qom-fuse under /machine, with irq and
> qemu:memory-region ommitted for brevity:
> 
>     machine  witherspoon-bmc-machine
>       +-- peripheral  container
>       |     +-- qdev-id  m25p80
>       +-- peripheral-anon  container
>       |     +-- device[0]  m25p80
>       +-- soc  ast2500-a1
>       |     +-- cpu  arm1176-arm-cpu
>       |     +-- fmc  aspeed.smc.ast2500-fmc
>       |     |     +-- spi  SSI
>       |     +-- ftgmac100  ftgmac100
>       |     +-- i2c  aspeed.i2c
>       |     |     +-- aspeed.i2c.0  i2c-bus
>       |     |     .
>       |     |     .   more i2c-bus
>       |     |     .
>       |     |     +-- aspeed.i2c.13  i2c-bus
>       |     +-- scu  aspeed.scu
>       |     +-- sdmc  aspeed.sdmc
>       |     +-- spi[0]  aspeed.smc.ast2500-spi1
>       |     |     +-- spi  SSI
>       |     +-- spi[1]  aspeed.smc.ast2500-spi2
>       |     |     +-- spi  SSI
>       |     +-- timerctrl  aspeed.timer
>       |     +-- vic  aspeed.vic
>       |     +-- wdt[0]  aspeed.wdt
>       |     +-- wdt[1]  aspeed.wdt
>       |     +-- wdt[2]  aspeed.wdt
>       +-- unattached  container
>             +-- device[0]  unimplemented-device
>             +-- device[1]  mx25l25635e
>             +-- device[2]  mx25l25635e
>             +-- device[3]  mx66l1g45g
>             +-- device[4]  pca9552
>             +-- device[5]  tmp423
>             +-- device[6]  tmp423
>             +-- device[7]  tmp105
>             +-- device[8]  ds1338
>             +-- device[9]  smbus-eeprom
>             +-- device[10]  pca9552
>             +-- sysbus  System
> 
> Observations (same as for ARM virt, more or less):
> 
> * Where ARM virt had its onboard components as direct children of
>   machine, witherspoon-bmc-machine has them wrapped in soc ast2500-a1.
> 
> * machine additionally has a few containers: peripheral,
>   peripheral-anon, unattached.
> 
> * machine/peripheral and machine/peripheral-anon contain the -device
>   with and without ID, respectively.
> 
> * machine/unattached contains everything else created by code without an
>   explicit parent device.  Some (all?) of them should perhaps be direct
>   children of machine or (unlike ARM virt) soc instead.
> 
> qdev tree shown by info qtree:
> 
>     bus: main-system-bus
>       type System
>       dev: unimplemented-device, id ""
>         size = 2097152 (0x200000)
>         name = "aspeed_soc.io"
>         mmio 000000001e600000/0000000000200000
>       dev: ftgmac100, id ""
>         gpio-out "sysbus-irq" 1
>         aspeed = true
>         mac = "52:54:00:12:34:56"
>         netdev = ""
>         mmio 000000001e660000/0000000000002000
>       dev: aspeed.wdt, id ""
>         silicon-rev = 67175171 (0x4010303)
>         mmio 000000001e785040/0000000000000020
>       dev: aspeed.wdt, id ""
>         silicon-rev = 67175171 (0x4010303)
>         mmio 000000001e785020/0000000000000020
>       dev: aspeed.wdt, id ""
>         silicon-rev = 67175171 (0x4010303)
>         mmio 000000001e785000/0000000000000020
>       dev: aspeed.sdmc, id ""
>         silicon-rev = 67175171 (0x4010303)
>         ram-size = 536870912 (0x20000000)
>         max-ram-size = 1073741824 (0x40000000)
>         mmio 000000001e6e0000/0000000000001000
>       dev: aspeed.smc.ast2500-spi2, id ""
>         gpio-out "sysbus-irq" 2
>         num-cs = 1 (0x1)
>         mmio 000000001e631000/0000000000000100
>         mmio 0000000038000000/0000000008000000
>         bus: spi
>           type SSI
>           dev: m25p80, id "qdev-id"
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>           dev: m25p80, id ""
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>       dev: aspeed.smc.ast2500-spi1, id ""
>         gpio-out "sysbus-irq" 2
>         num-cs = 1 (0x1)
>         mmio 000000001e630000/0000000000000100
>         mmio 0000000030000000/0000000008000000
>         bus: spi
>           type SSI
>           dev: mx66l1g45g, id ""
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>       dev: aspeed.smc.ast2500-fmc, id ""
>         gpio-out "sysbus-irq" 3
>         num-cs = 2 (0x2)
>         mmio 000000001e620000/0000000000000100
>         mmio 0000000020000000/0000000010000000
>         bus: spi
>           type SSI
>           dev: mx25l25635e, id ""
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>           dev: mx25l25635e, id ""
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>       dev: aspeed.i2c, id ""
>         gpio-out "sysbus-irq" 1
>         mmio 000000001e78a000/0000000000001000
>         bus: aspeed.i2c.13
>           type i2c-bus
>         ... more i2c-bus
>         bus: aspeed.i2c.0
>           type i2c-bus
>       dev: aspeed.timer, id ""
>         gpio-out "sysbus-irq" 8
>         mmio 000000001e782000/0000000000001000
>       dev: aspeed.vic, id ""
>         gpio-out "sysbus-irq" 2
>         gpio-in "" 51
>         mmio 000000001e6c0000/0000000000020000
>       dev: aspeed.scu, id ""
>         silicon-rev = 67175171 (0x4010303)
>         hw-strap1 = 4044018182 (0xf10ad206)
>         hw-strap2 = 0 (0x0)
>         hw-prot-key = 0 (0x0)
>         mmio 000000001e6e2000/0000000000001000
> 
> Observations (same as for ARM virt):
> 
> * machine's containers are not in the qtree.
> 
> * Composition tree node arm1176-arm-cpu is not in the qtree.  That's
>   because it isn't connected to a qbus.
> 
>   Same for pca9552, tmp423, tmp105, ds1338, smbus-eeprom, I guess.

That is odd:

static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
{
    AspeedSoCState *soc = &bmc->soc;

    i2c_create_slave(
        aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
        TYPE_PCA9552, 0x60);
    ...

DeviceState *i2c_create_slave(I2CBus *bus, const char *name, ...
{
    DeviceState *dev;

    dev = qdev_create(&bus->qbus, name);
    ...

static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
{
    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
    AspeedI2CState *s = ASPEED_I2C(dev);

    for (i = 0; i < ASPEED_I2C_NR_BUSSES; i++) {
        s->busses[i].bus = i2c_init_bus(dev, name);
        ...

I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
{
    I2CBus *bus;

    bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name));
    ...

static const TypeInfo i2c_bus_info = {
    .name = TYPE_I2C_BUS,
    .parent = TYPE_BUS,
    ...

> * In the qtree, every other inner node is a qbus.  These are *leaves* in
>   the composition tree.  The qtree's vertex from qbus to qdev is a
>   *link* in the composition tree.
> 
>   Example: main-system-bus -> scu is
>       machine/unattached/sysbus/child[0] ->
>       ../../../machine/soc/scu.
> 
>   Example: main-system-bus -> unimplemented-device is
>       machine/unattached/sysbus/child[12] ->
>       ../../../machine/unattached/device[12].
> 
>   Example: main-system-bus/aspeed.smc.ast2500-spi1/spi -> mx66l1g45g is
>       machine/soc/spi\[0\]/spi/child[0] ->
>       ../../../../machine/unattached/device[3].
> 
>   Example: main-system-bus/aspeed.smc.ast2500-spi2/spi -> m25p80
>       (the one without a qdev ID) is
>       machine/soc/spi\[1\]/spi/child[0] ->
>       ../../../../machine/peripheral-anon/device[0]
> 
>   Example: main-system-bus/aspeed.smc.ast2500-spi2/spi -> m25p80
>       (the one with qdev ID "qdev-id") is
>       machine/soc/spi\[1\]/spi/child[1] ->
>       ../../../../machine/peripheral/qdev-id
>