* [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn @ 2019-05-24 17:54 Philippe Mathieu-Daudé 2019-05-24 18:04 ` David Hildenbrand 0 siblings, 1 reply; 18+ messages in thread From: Philippe Mathieu-Daudé @ 2019-05-24 17:54 UTC (permalink / raw) To: Christian Borntraeger, Markus Armbruster Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, QEMU Developers, Halil Pasic, qemu-s390x Hi Christian, I'm having hard time to understand why the S390_IPL object calls qemu_register_reset(qdev_reset_all_fn) in its realize() method, while being QOM'ified (it has a reset method). It doesn't seem to have a qdev children added explicitly to it. I see it is used as a singleton, what else am I missing? Thanks, Phil. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 17:54 [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn Philippe Mathieu-Daudé @ 2019-05-24 18:04 ` David Hildenbrand 2019-05-24 18:20 ` Philippe Mathieu-Daudé 2019-05-24 18:28 ` Christian Borntraeger 0 siblings, 2 replies; 18+ messages in thread From: David Hildenbrand @ 2019-05-24 18:04 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Christian Borntraeger, Markus Armbruster Cc: Halil Pasic, Thomas Huth, Cornelia Huck, QEMU Developers, qemu-s390x On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: > Hi Christian, > > I'm having hard time to understand why the S390_IPL object calls > qemu_register_reset(qdev_reset_all_fn) in its realize() method, while > being QOM'ified (it has a reset method). > > It doesn't seem to have a qdev children added explicitly to it. > I see it is used as a singleton, what else am I missing? > > Thanks, > > Phil. > Looks like I added it back then (~4 years ago) when converting it into a TYPE_DEVICE. I could imagine that - back then - this was needed because only TYPE_SYS_BUS_DEVICE would recursively get reset. Did you try removing it, to see if anything breaks? -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 18:04 ` David Hildenbrand @ 2019-05-24 18:20 ` Philippe Mathieu-Daudé 2019-05-24 18:28 ` Christian Borntraeger 1 sibling, 0 replies; 18+ messages in thread From: Philippe Mathieu-Daudé @ 2019-05-24 18:20 UTC (permalink / raw) To: David Hildenbrand, Christian Borntraeger, Markus Armbruster Cc: Halil Pasic, Thomas Huth, Cornelia Huck, QEMU Developers, qemu-s390x On 5/24/19 8:04 PM, David Hildenbrand wrote: > On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: >> Hi Christian, >> >> I'm having hard time to understand why the S390_IPL object calls >> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while >> being QOM'ified (it has a reset method). >> >> It doesn't seem to have a qdev children added explicitly to it. >> I see it is used as a singleton, what else am I missing? >> >> Thanks, >> >> Phil. >> > > Looks like I added it back then (~4 years ago) when converting it into a > TYPE_DEVICE. > > I could imagine that - back then - this was needed because only > TYPE_SYS_BUS_DEVICE would recursively get reset. Thanks for the quick response :) > Did you try removing it, to see if anything breaks? From build POV it is OK, but I have now idea of the effects with KVM. I don't know how to test on s390x systems, but luckily Patchew/s390x is back up so I'll try my luck with a RFC patch, but I'm not sure the default tests cover this specific device uses. Regards, Phil. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 18:04 ` David Hildenbrand 2019-05-24 18:20 ` Philippe Mathieu-Daudé @ 2019-05-24 18:28 ` Christian Borntraeger 2019-05-24 18:36 ` David Hildenbrand 1 sibling, 1 reply; 18+ messages in thread From: Christian Borntraeger @ 2019-05-24 18:28 UTC (permalink / raw) To: David Hildenbrand, Philippe Mathieu-Daudé, Markus Armbruster Cc: Halil Pasic, Thomas Huth, Cornelia Huck, QEMU Developers, qemu-s390x On 24.05.19 20:04, David Hildenbrand wrote: > On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: >> Hi Christian, >> >> I'm having hard time to understand why the S390_IPL object calls >> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while >> being QOM'ified (it has a reset method). >> >> It doesn't seem to have a qdev children added explicitly to it. >> I see it is used as a singleton, what else am I missing? >> >> Thanks, >> >> Phil. >> > > Looks like I added it back then (~4 years ago) when converting it into a > TYPE_DEVICE. > > I could imagine that - back then - this was needed because only > TYPE_SYS_BUS_DEVICE would recursively get reset. Yes, back then singleton devices were not recursively resetted. Has that changed? > > Did you try removing it, to see if anything breaks? > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 18:28 ` Christian Borntraeger @ 2019-05-24 18:36 ` David Hildenbrand 2019-05-24 19:00 ` David Hildenbrand 0 siblings, 1 reply; 18+ messages in thread From: David Hildenbrand @ 2019-05-24 18:36 UTC (permalink / raw) To: Christian Borntraeger, Philippe Mathieu-Daudé, Markus Armbruster Cc: Halil Pasic, Thomas Huth, Cornelia Huck, QEMU Developers, qemu-s390x On 24.05.19 20:28, Christian Borntraeger wrote: > > > On 24.05.19 20:04, David Hildenbrand wrote: >> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: >>> Hi Christian, >>> >>> I'm having hard time to understand why the S390_IPL object calls >>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while >>> being QOM'ified (it has a reset method). >>> >>> It doesn't seem to have a qdev children added explicitly to it. >>> I see it is used as a singleton, what else am I missing? >>> >>> Thanks, >>> >>> Phil. >>> >> >> Looks like I added it back then (~4 years ago) when converting it into a >> TYPE_DEVICE. >> >> I could imagine that - back then - this was needed because only >> TYPE_SYS_BUS_DEVICE would recursively get reset. > > Yes, back then singleton devices were not recursively resetted. Has that changed? Hacking that call out, I don't see it getting called anymore. So it is still required. The question is if it can be reworked. -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 18:36 ` David Hildenbrand @ 2019-05-24 19:00 ` David Hildenbrand 2019-05-24 19:45 ` Christian Borntraeger 0 siblings, 1 reply; 18+ messages in thread From: David Hildenbrand @ 2019-05-24 19:00 UTC (permalink / raw) To: Christian Borntraeger, Philippe Mathieu-Daudé, Markus Armbruster Cc: Halil Pasic, Thomas Huth, Cornelia Huck, QEMU Developers, qemu-s390x On 24.05.19 20:36, David Hildenbrand wrote: > On 24.05.19 20:28, Christian Borntraeger wrote: >> >> >> On 24.05.19 20:04, David Hildenbrand wrote: >>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: >>>> Hi Christian, >>>> >>>> I'm having hard time to understand why the S390_IPL object calls >>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while >>>> being QOM'ified (it has a reset method). >>>> >>>> It doesn't seem to have a qdev children added explicitly to it. >>>> I see it is used as a singleton, what else am I missing? >>>> >>>> Thanks, >>>> >>>> Phil. >>>> >>> >>> Looks like I added it back then (~4 years ago) when converting it into a >>> TYPE_DEVICE. >>> >>> I could imagine that - back then - this was needed because only >>> TYPE_SYS_BUS_DEVICE would recursively get reset. >> >> Yes, back then singleton devices were not recursively resetted. Has that changed? > > Hacking that call out, I don't see it getting called anymore. So it is > still required. The question is if it can be reworked. > Yes, as it is not a sysbus device, it won't get reset. The owner (machine) has to take care of this. The following works: diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index b93750c14e..91a31c2cd0 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) */ ipl->compat_start_addr = ipl->start_addr; ipl->compat_bios_start_addr = ipl->bios_start_addr; - qemu_register_reset(qdev_reset_all_fn, dev); error: error_propagate(errp, err); } diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index bbc6e8fa0b..658ab529a1 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); } +static void s390_ipl_reset(void) +{ + qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL))); +} + static void s390_machine_reset(void) { enum s390_reset reset_type; @@ -353,6 +358,7 @@ static void s390_machine_reset(void) case S390_RESET_EXTERNAL: case S390_RESET_REIPL: qemu_devices_reset(); + s390_ipl_reset(); s390_crypto_reset(); /* configure and start the ipl CPU only */ -- Thanks, David / dhildenb ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 19:00 ` David Hildenbrand @ 2019-05-24 19:45 ` Christian Borntraeger 2019-05-24 19:58 ` David Hildenbrand ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Christian Borntraeger @ 2019-05-24 19:45 UTC (permalink / raw) To: David Hildenbrand, Philippe Mathieu-Daudé, Markus Armbruster Cc: Halil Pasic, Thomas Huth, Cornelia Huck, QEMU Developers, qemu-s390x On 24.05.19 21:00, David Hildenbrand wrote: > On 24.05.19 20:36, David Hildenbrand wrote: >> On 24.05.19 20:28, Christian Borntraeger wrote: >>> >>> >>> On 24.05.19 20:04, David Hildenbrand wrote: >>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: >>>>> Hi Christian, >>>>> >>>>> I'm having hard time to understand why the S390_IPL object calls >>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while >>>>> being QOM'ified (it has a reset method). >>>>> >>>>> It doesn't seem to have a qdev children added explicitly to it. >>>>> I see it is used as a singleton, what else am I missing? >>>>> >>>>> Thanks, >>>>> >>>>> Phil. >>>>> >>>> >>>> Looks like I added it back then (~4 years ago) when converting it into a >>>> TYPE_DEVICE. >>>> >>>> I could imagine that - back then - this was needed because only >>>> TYPE_SYS_BUS_DEVICE would recursively get reset. >>> >>> Yes, back then singleton devices were not recursively resetted. Has that changed? >> >> Hacking that call out, I don't see it getting called anymore. So it is >> still required. The question is if it can be reworked. >> > > Yes, as it is not a sysbus device, it won't get reset. > The owner (machine) has to take care of this. The following works: > > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index b93750c14e..91a31c2cd0 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > */ > ipl->compat_start_addr = ipl->start_addr; > ipl->compat_bios_start_addr = ipl->bios_start_addr; > - qemu_register_reset(qdev_reset_all_fn, dev); > error: > error_propagate(errp, err); > } > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index bbc6e8fa0b..658ab529a1 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) > s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > } > > +static void s390_ipl_reset(void) > +{ > + qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL))); > +} > + > static void s390_machine_reset(void) > { > enum s390_reset reset_type; > @@ -353,6 +358,7 @@ static void s390_machine_reset(void) > case S390_RESET_EXTERNAL: > case S390_RESET_REIPL: > qemu_devices_reset(); > + s390_ipl_reset(); > s390_crypto_reset(); > > /* configure and start the ipl CPU only */ > While this patch is certainly ok, I find it disturbing that qdev devices are being resetted, but qom devices not. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 19:45 ` Christian Borntraeger @ 2019-05-24 19:58 ` David Hildenbrand 2019-05-25 15:03 ` Peter Maydell 2019-05-28 8:29 ` David Hildenbrand 2 siblings, 0 replies; 18+ messages in thread From: David Hildenbrand @ 2019-05-24 19:58 UTC (permalink / raw) To: Christian Borntraeger, Philippe Mathieu-Daudé, Markus Armbruster Cc: Halil Pasic, Thomas Huth, Cornelia Huck, QEMU Developers, qemu-s390x On 24.05.19 21:45, Christian Borntraeger wrote: > > > On 24.05.19 21:00, David Hildenbrand wrote: >> On 24.05.19 20:36, David Hildenbrand wrote: >>> On 24.05.19 20:28, Christian Borntraeger wrote: >>>> >>>> >>>> On 24.05.19 20:04, David Hildenbrand wrote: >>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: >>>>>> Hi Christian, >>>>>> >>>>>> I'm having hard time to understand why the S390_IPL object calls >>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while >>>>>> being QOM'ified (it has a reset method). >>>>>> >>>>>> It doesn't seem to have a qdev children added explicitly to it. >>>>>> I see it is used as a singleton, what else am I missing? >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Phil. >>>>>> >>>>> >>>>> Looks like I added it back then (~4 years ago) when converting it into a >>>>> TYPE_DEVICE. >>>>> >>>>> I could imagine that - back then - this was needed because only >>>>> TYPE_SYS_BUS_DEVICE would recursively get reset. >>>> >>>> Yes, back then singleton devices were not recursively resetted. Has that changed? >>> >>> Hacking that call out, I don't see it getting called anymore. So it is >>> still required. The question is if it can be reworked. >>> >> >> Yes, as it is not a sysbus device, it won't get reset. >> The owner (machine) has to take care of this. The following works: >> >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index b93750c14e..91a31c2cd0 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) >> */ >> ipl->compat_start_addr = ipl->start_addr; >> ipl->compat_bios_start_addr = ipl->bios_start_addr; >> - qemu_register_reset(qdev_reset_all_fn, dev); >> error: >> error_propagate(errp, err); >> } >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index bbc6e8fa0b..658ab529a1 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >> } >> >> +static void s390_ipl_reset(void) >> +{ >> + qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL))); >> +} >> + >> static void s390_machine_reset(void) >> { >> enum s390_reset reset_type; >> @@ -353,6 +358,7 @@ static void s390_machine_reset(void) >> case S390_RESET_EXTERNAL: >> case S390_RESET_REIPL: >> qemu_devices_reset(); >> + s390_ipl_reset(); >> s390_crypto_reset(); >> >> /* configure and start the ipl CPU only */ >> > > While this patch is certainly ok, I find it disturbing that qdev devices are being resetted, > but qom devices not. I guess the disturbing thing is "... after all these years" :) -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 19:45 ` Christian Borntraeger 2019-05-24 19:58 ` David Hildenbrand @ 2019-05-25 15:03 ` Peter Maydell 2019-05-27 7:52 ` Markus Armbruster 2019-05-28 8:29 ` David Hildenbrand 2 siblings, 1 reply; 18+ messages in thread From: Peter Maydell @ 2019-05-25 15:03 UTC (permalink / raw) To: Christian Borntraeger Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, Markus Armbruster, QEMU Developers, Halil Pasic, qemu-s390x, Philippe Mathieu-Daudé On Fri, 24 May 2019 at 20:47, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > While this patch is certainly ok, I find it disturbing that qdev devices are being resetted, > but qom devices not. It's not a qdev-vs-QOM thing. Anything which is a DeviceState has a reset method, but only devices which are somewhere rooted in the bus-tree that starts with the "main system bus" (aka sysbus) get reset by the vl.c-registered "reset everything on the system bus". Devices which are SysBusDevice get auto-parented onto the sysbus, and so get reset. Devices like PCI devices or SCSI devices get put onto the PCI bus or the SCSI bus, and those buses are in turn children of some host-controller device which is on the sysbus, so they all get reset. The things that don't get reset are "orphan" devices which are neither (a) of a type that gets automatically parented onto a bus like SysBusDevice nor (b) put specifically onto some other bus. CPU objects are the other common thing that doesn't get reset 'automatically'. 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). thanks -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-25 15:03 ` Peter Maydell @ 2019-05-27 7:52 ` Markus Armbruster 2019-05-27 9:59 ` Philippe Mathieu-Daudé 2019-05-27 18:55 ` Peter Maydell 0 siblings, 2 replies; 18+ messages in thread From: Markus Armbruster @ 2019-05-27 7:52 UTC (permalink / raw) To: Peter Maydell Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, QEMU Developers, Halil Pasic, Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 24 May 2019 at 20:47, Christian Borntraeger > <borntraeger@de.ibm.com> wrote: >> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted, >> but qom devices not. > > It's not a qdev-vs-QOM thing. Anything which is a DeviceState > has a reset method, but only devices which are somewhere > rooted in the bus-tree that starts with the "main system > bus" (aka sysbus) get reset by the vl.c-registered "reset > everything on the system bus". Devices which are SysBusDevice > get auto-parented onto the sysbus, and so get reset. Devices > like PCI devices or SCSI devices get put onto the PCI > bus or the SCSI bus, and those buses are in turn children > of some host-controller device which is on the sysbus, so > they all get reset. The things that don't get reset are > "orphan" devices which are neither (a) of a type that gets > automatically parented onto a bus like SysBusDevice nor > (b) put specifically onto some other bus. > > CPU objects are the other common thing that doesn't get > reset 'automatically'. > > 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: machine virt-4.1-machine +-- fw_cfg fw_cfg_mem +-- peripheral container +-- peripheral-anon container | +-- device[0] e1000 +-- unattached container | +-- device[0] cortex-a15-arm-cpu | +-- device[1] arm_gic | +-- device[2] arm-gicv2m | +-- device[3] pl011 | +-- device[4] pl031 | +-- device[5] gpex-pcihost | | +-- pcie.0 PCIE | | +-- gpex_root gpex-root | +-- device[6] pl061 | +-- device[7] gpio-key | +-- device[8] virtio-mmio | | +-- virtio-mmio-bus.0 virtio-mmio-bus | . | . more virtio-mmio | . | +-- device[39] virtio-mmio | | +-- virtio-mmio-bus.31 virtio-mmio-bus | +-- device[40] platform-bus-device | +-- sysbus System +-- virt.flash0 cfi.pflash01 +-- virt.flash1 cfi.pflash01 Observations: * Some components of the machine are direct children of machine: fw_cfg, virt.flash0, virt.flash1 * 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 instead. Compare to the qdev tree shown by info qtree: bus: main-system-bus type System dev: platform-bus-device, id "platform-bus-device" dev: fw_cfg_mem, id "" dev: virtio-mmio, id "" bus: virtio-mmio-bus.31 type virtio-mmio-bus ... more virtio-mmio dev: virtio-mmio, id "" bus: virtio-mmio-bus.0 type virtio-mmio-bus dev: gpio-key, id "" dev: pl061, id "" dev: gpex-pcihost, id "" bus: pcie.0 type PCIE dev: e1000, id "" dev: gpex-root, id "" dev: pl031, id "" dev: pl011, id "" dev: arm-gicv2m, id "" dev: arm_gic, id "" dev: cfi.pflash01, id "" dev: cfi.pflash01, id "" Observations: * Composition tree root machine's containers are not in the qtree. * Composition tree node cortex-a15-arm-cpu is not in the qtree. That's because it's not a qdev (in QOM parlance: not a TYPE_DEVICE). * 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 -> pl011 is machine/unattached/sysbus/child[4] -> ../../../machine/unattached/device[3]. Example: main-system-bus/gpex-pcihost/pcie.0 -> e1000 is machine/unattached/device[5]/pcie.0//child[1] -> ../../../../machine/peripheral-anon/device[0]. Now let me ramble a bit on reset. We could model the reset wiring explicitly: every QOM object that wants to participate in reset has a reset input pin. We represent the wiring as links. The reset links form a reset tree. Example: object virt-4.1-machine at machine gets a link reset[4] pointing to its child object cfi.pflash01 at machine/virt.flash0. Example: object PCIE at machine/unattached/device[5]/pcie.0 gets a link reset[1] pointing to object e1000 at machine/peripheral-anon/device[0]. Note that reset[1] points to the same object as child[1]. Adding all these links in code would be tiresome. Enter defaults. If an object doesn't get its reset input connected, and * the parent is machine/peripheral or machine/peripheral-anon, default to the object that links to it in the composition tree. Error when there's more than one. Example: object e1000 at machine/peripheral-anon/device[0] defaults to machine/unattached/device[5]/pcie.0. * the parent is machine/unattached, default to machine. Example: object gpex-pcihost at machine/unattached/device[5] defaults to machine. * the parent is not one of these containers, default to its parent in the composition tree. Example: object cfi.pflash01 at machine/virt.flash0 defaults to machine. This leaves the order in which an object asserts its reset wires unspecified. In physical hardware, these guys get asserted simultaneously, and the various pieces of silicon at the other ends reset in parallel. In software, we serialize. We could pick some order by numbering the reset links, say reset[0], reset[1], ... Reset could then work by walking the reset tree, running device model callbacks before and after walking the children. Cases may well exist where reset is more complicated than that. Perhaps an object resets its children in the reset tree at different times during its own reset. Such cases need to modelled in device model code. That's another callback. This is quite possibly too naive to be practical. Hey, you asked :) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-27 7:52 ` Markus Armbruster @ 2019-05-27 9:59 ` Philippe Mathieu-Daudé 2019-05-27 18:55 ` Peter Maydell 1 sibling, 0 replies; 18+ messages in thread From: Philippe Mathieu-Daudé @ 2019-05-27 9:59 UTC (permalink / raw) To: Markus Armbruster, Peter Maydell Cc: Damien Hedde, Edgar Iglesias, Thomas Huth, David Hildenbrand, Cornelia Huck, QEMU Developers, Halil Pasic, Christian Borntraeger, qemu-s390x, Luc Michel Cc'ing Damien who is working on "multi-phase reset mechanism". On 5/27/19 9:52 AM, Markus Armbruster wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Fri, 24 May 2019 at 20:47, Christian Borntraeger >> <borntraeger@de.ibm.com> wrote: >>> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted, >>> but qom devices not. >> >> It's not a qdev-vs-QOM thing. Anything which is a DeviceState >> has a reset method, but only devices which are somewhere >> rooted in the bus-tree that starts with the "main system >> bus" (aka sysbus) get reset by the vl.c-registered "reset >> everything on the system bus". Devices which are SysBusDevice >> get auto-parented onto the sysbus, and so get reset. Devices >> like PCI devices or SCSI devices get put onto the PCI >> bus or the SCSI bus, and those buses are in turn children >> of some host-controller device which is on the sysbus, so >> they all get reset. The things that don't get reset are >> "orphan" devices which are neither (a) of a type that gets >> automatically parented onto a bus like SysBusDevice nor >> (b) put specifically onto some other bus. >> >> CPU objects are the other common thing that doesn't get >> reset 'automatically'. >> >> 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: > > machine virt-4.1-machine > +-- fw_cfg fw_cfg_mem > +-- peripheral container > +-- peripheral-anon container > | +-- device[0] e1000 > +-- unattached container > | +-- device[0] cortex-a15-arm-cpu > | +-- device[1] arm_gic > | +-- device[2] arm-gicv2m > | +-- device[3] pl011 > | +-- device[4] pl031 > | +-- device[5] gpex-pcihost > | | +-- pcie.0 PCIE > | | +-- gpex_root gpex-root > | +-- device[6] pl061 > | +-- device[7] gpio-key > | +-- device[8] virtio-mmio > | | +-- virtio-mmio-bus.0 virtio-mmio-bus > | . > | . more virtio-mmio > | . > | +-- device[39] virtio-mmio > | | +-- virtio-mmio-bus.31 virtio-mmio-bus > | +-- device[40] platform-bus-device > | +-- sysbus System > +-- virt.flash0 cfi.pflash01 > +-- virt.flash1 cfi.pflash01 > > Observations: > > * Some components of the machine are direct children of machine: fw_cfg, > virt.flash0, virt.flash1 > > * 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 instead. > > Compare to the qdev tree shown by info qtree: > > bus: main-system-bus > type System > dev: platform-bus-device, id "platform-bus-device" > dev: fw_cfg_mem, id "" > dev: virtio-mmio, id "" > bus: virtio-mmio-bus.31 > type virtio-mmio-bus > ... more virtio-mmio > dev: virtio-mmio, id "" > bus: virtio-mmio-bus.0 > type virtio-mmio-bus > dev: gpio-key, id "" > dev: pl061, id "" > dev: gpex-pcihost, id "" > bus: pcie.0 > type PCIE > dev: e1000, id "" > dev: gpex-root, id "" > dev: pl031, id "" > dev: pl011, id "" > dev: arm-gicv2m, id "" > dev: arm_gic, id "" > dev: cfi.pflash01, id "" > dev: cfi.pflash01, id "" > > Observations: > > * Composition tree root machine's containers are not in the qtree. > > * Composition tree node cortex-a15-arm-cpu is not in the qtree. That's > because it's not a qdev (in QOM parlance: not a TYPE_DEVICE). > > * 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 -> pl011 is > machine/unattached/sysbus/child[4] -> > ../../../machine/unattached/device[3]. > > Example: main-system-bus/gpex-pcihost/pcie.0 -> e1000 is > machine/unattached/device[5]/pcie.0//child[1] -> > ../../../../machine/peripheral-anon/device[0]. > > Now let me ramble a bit on reset. > > We could model the reset wiring explicitly: every QOM object that wants > to participate in reset has a reset input pin. We represent the wiring > as links. The reset links form a reset tree. > > Example: object virt-4.1-machine at machine gets a link reset[4] > pointing to its child object cfi.pflash01 at machine/virt.flash0. > > Example: object PCIE at machine/unattached/device[5]/pcie.0 gets a link > reset[1] pointing to object e1000 at > machine/peripheral-anon/device[0]. > > Note that reset[1] points to the same object as child[1]. > > Adding all these links in code would be tiresome. Enter defaults. > > If an object doesn't get its reset input connected, and > > * the parent is machine/peripheral or machine/peripheral-anon, default > to the object that links to it in the composition tree. Error when > there's more than one. > > Example: object e1000 at machine/peripheral-anon/device[0] defaults to > machine/unattached/device[5]/pcie.0. > > * the parent is machine/unattached, default to machine. > > Example: object gpex-pcihost at machine/unattached/device[5] defaults > to machine. > > * the parent is not one of these containers, default to its parent in > the composition tree. > > Example: object cfi.pflash01 at machine/virt.flash0 defaults to > machine. > > This leaves the order in which an object asserts its reset wires > unspecified. In physical hardware, these guys get asserted > simultaneously, and the various pieces of silicon at the other ends > reset in parallel. In software, we serialize. > > We could pick some order by numbering the reset links, say reset[0], > reset[1], ... > > Reset could then work by walking the reset tree, running device model > callbacks before and after walking the children. > > Cases may well exist where reset is more complicated than that. Perhaps > an object resets its children in the reset tree at different times > during its own reset. Such cases need to modelled in device model code. > That's another callback. > > This is quite possibly too naive to be practical. Hey, you asked :) Thanks a lot for this extensive analysis :) I guess you indirectly answered to some of the issues Peter pointed in https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg03667.html Markus, if your bandwidth allows it, you might want to have a look at it :) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-27 7:52 ` Markus Armbruster 2019-05-27 9:59 ` Philippe Mathieu-Daudé @ 2019-05-27 18:55 ` Peter Maydell 2019-05-28 5:02 ` Markus Armbruster 2019-05-29 6:08 ` Markus Armbruster 1 sibling, 2 replies; 18+ messages in thread From: Peter Maydell @ 2019-05-27 18:55 UTC (permalink / raw) To: Markus Armbruster Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, QEMU Developers, Halil Pasic, Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé On Mon, 27 May 2019 at 08:52, Markus Armbruster <armbru@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> 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). > Observations: > > * Composition tree root machine's containers are not in the qtree. > > * Composition tree node cortex-a15-arm-cpu is not in the qtree. That's > because it's not a qdev (in QOM parlance: not a TYPE_DEVICE). Hmm? The Arm CPUs all subclass CPUClass, which subclasses DeviceState. The CPU is a qdev, but it is not in the qtree because it does not have a bus it can live on. > Now let me ramble a bit on reset. Thanks for this -- I have put this on my list to think through in detail next week. -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-27 18:55 ` Peter Maydell @ 2019-05-28 5:02 ` Markus Armbruster 2019-05-29 6:08 ` Markus Armbruster 1 sibling, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2019-05-28 5:02 UTC (permalink / raw) To: Peter Maydell Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, QEMU Developers, Halil Pasic, Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 27 May 2019 at 08:52, Markus Armbruster <armbru@redhat.com> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> 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). Can do. >> Observations: >> >> * Composition tree root machine's containers are not in the qtree. >> >> * Composition tree node cortex-a15-arm-cpu is not in the qtree. That's >> because it's not a qdev (in QOM parlance: not a TYPE_DEVICE). > > Hmm? The Arm CPUs all subclass CPUClass, which subclasses > DeviceState. The CPU is a qdev, but it is not in the qtree because > it does not have a bus it can live on. You're right. >> Now let me ramble a bit on reset. > > Thanks for this -- I have put this on my list to > think through in detail next week. Sounds good. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-27 18:55 ` Peter Maydell 2019-05-28 5:02 ` Markus Armbruster @ 2019-05-29 6:08 ` Markus Armbruster 2019-05-29 10:32 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 18+ messages in thread From: Markus Armbruster @ 2019-05-29 6:08 UTC (permalink / raw) To: Peter Maydell Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, QEMU Developers, Halil Pasic, Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 27 May 2019 at 08:52, Markus Armbruster <armbru@redhat.com> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> 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. * 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-29 6:08 ` Markus Armbruster @ 2019-05-29 10:32 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 18+ messages in thread From: Philippe Mathieu-Daudé @ 2019-05-29 10:32 UTC (permalink / raw) To: Markus Armbruster, Peter Maydell Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, QEMU Developers, Halil Pasic, Christian Borntraeger, qemu-s390x On 5/29/19 8:08 AM, Markus Armbruster wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Mon, 27 May 2019 at 08:52, Markus Armbruster <armbru@redhat.com> wrote: >>> >>> Peter Maydell <peter.maydell@linaro.org> 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-24 19:45 ` Christian Borntraeger 2019-05-24 19:58 ` David Hildenbrand 2019-05-25 15:03 ` Peter Maydell @ 2019-05-28 8:29 ` David Hildenbrand 2019-05-28 8:33 ` Cornelia Huck 2 siblings, 1 reply; 18+ messages in thread From: David Hildenbrand @ 2019-05-28 8:29 UTC (permalink / raw) To: Christian Borntraeger, Philippe Mathieu-Daudé, Markus Armbruster Cc: Halil Pasic, Thomas Huth, Cornelia Huck, QEMU Developers, qemu-s390x On 24.05.19 21:45, Christian Borntraeger wrote: > > > On 24.05.19 21:00, David Hildenbrand wrote: >> On 24.05.19 20:36, David Hildenbrand wrote: >>> On 24.05.19 20:28, Christian Borntraeger wrote: >>>> >>>> >>>> On 24.05.19 20:04, David Hildenbrand wrote: >>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: >>>>>> Hi Christian, >>>>>> >>>>>> I'm having hard time to understand why the S390_IPL object calls >>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while >>>>>> being QOM'ified (it has a reset method). >>>>>> >>>>>> It doesn't seem to have a qdev children added explicitly to it. >>>>>> I see it is used as a singleton, what else am I missing? >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Phil. >>>>>> >>>>> >>>>> Looks like I added it back then (~4 years ago) when converting it into a >>>>> TYPE_DEVICE. >>>>> >>>>> I could imagine that - back then - this was needed because only >>>>> TYPE_SYS_BUS_DEVICE would recursively get reset. >>>> >>>> Yes, back then singleton devices were not recursively resetted. Has that changed? >>> >>> Hacking that call out, I don't see it getting called anymore. So it is >>> still required. The question is if it can be reworked. >>> >> >> Yes, as it is not a sysbus device, it won't get reset. >> The owner (machine) has to take care of this. The following works: >> >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index b93750c14e..91a31c2cd0 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) >> */ >> ipl->compat_start_addr = ipl->start_addr; >> ipl->compat_bios_start_addr = ipl->bios_start_addr; >> - qemu_register_reset(qdev_reset_all_fn, dev); >> error: >> error_propagate(errp, err); >> } >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index bbc6e8fa0b..658ab529a1 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >> } >> >> +static void s390_ipl_reset(void) >> +{ >> + qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL))); >> +} >> + >> static void s390_machine_reset(void) >> { >> enum s390_reset reset_type; >> @@ -353,6 +358,7 @@ static void s390_machine_reset(void) >> case S390_RESET_EXTERNAL: >> case S390_RESET_REIPL: >> qemu_devices_reset(); >> + s390_ipl_reset(); >> s390_crypto_reset(); >> >> /* configure and start the ipl CPU only */ >> > > While this patch is certainly ok, I find it disturbing that qdev devices are being resetted, > but qom devices not. > Shall I send that as a proper patch, or do we want to stick to the existing approach until we have improved the general reset approach? -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-28 8:29 ` David Hildenbrand @ 2019-05-28 8:33 ` Cornelia Huck 2019-05-28 9:29 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 18+ messages in thread From: Cornelia Huck @ 2019-05-28 8:33 UTC (permalink / raw) To: David Hildenbrand Cc: Thomas Huth, Markus Armbruster, QEMU Developers, Halil Pasic, Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé On Tue, 28 May 2019 10:29:09 +0200 David Hildenbrand <david@redhat.com> wrote: > On 24.05.19 21:45, Christian Borntraeger wrote: > > > > > > On 24.05.19 21:00, David Hildenbrand wrote: > >> On 24.05.19 20:36, David Hildenbrand wrote: > >>> On 24.05.19 20:28, Christian Borntraeger wrote: > >>>> > >>>> > >>>> On 24.05.19 20:04, David Hildenbrand wrote: > >>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: > >>>>>> Hi Christian, > >>>>>> > >>>>>> I'm having hard time to understand why the S390_IPL object calls > >>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while > >>>>>> being QOM'ified (it has a reset method). > >>>>>> > >>>>>> It doesn't seem to have a qdev children added explicitly to it. > >>>>>> I see it is used as a singleton, what else am I missing? > >>>>>> > >>>>>> Thanks, > >>>>>> > >>>>>> Phil. > >>>>>> > >>>>> > >>>>> Looks like I added it back then (~4 years ago) when converting it into a > >>>>> TYPE_DEVICE. > >>>>> > >>>>> I could imagine that - back then - this was needed because only > >>>>> TYPE_SYS_BUS_DEVICE would recursively get reset. > >>>> > >>>> Yes, back then singleton devices were not recursively resetted. Has that changed? > >>> > >>> Hacking that call out, I don't see it getting called anymore. So it is > >>> still required. The question is if it can be reworked. > >>> > >> > >> Yes, as it is not a sysbus device, it won't get reset. > >> The owner (machine) has to take care of this. The following works: > >> > >> > >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > >> index b93750c14e..91a31c2cd0 100644 > >> --- a/hw/s390x/ipl.c > >> +++ b/hw/s390x/ipl.c > >> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > >> */ > >> ipl->compat_start_addr = ipl->start_addr; > >> ipl->compat_bios_start_addr = ipl->bios_start_addr; > >> - qemu_register_reset(qdev_reset_all_fn, dev); > >> error: > >> error_propagate(errp, err); > >> } > >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > >> index bbc6e8fa0b..658ab529a1 100644 > >> --- a/hw/s390x/s390-virtio-ccw.c > >> +++ b/hw/s390x/s390-virtio-ccw.c > >> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) > >> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); > >> } > >> > >> +static void s390_ipl_reset(void) > >> +{ > >> + qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL))); > >> +} > >> + > >> static void s390_machine_reset(void) > >> { > >> enum s390_reset reset_type; > >> @@ -353,6 +358,7 @@ static void s390_machine_reset(void) > >> case S390_RESET_EXTERNAL: > >> case S390_RESET_REIPL: > >> qemu_devices_reset(); > >> + s390_ipl_reset(); > >> s390_crypto_reset(); > >> > >> /* configure and start the ipl CPU only */ > >> > > > > While this patch is certainly ok, I find it disturbing that qdev devices are being resetted, > > but qom devices not. > > > > Shall I send that as a proper patch, or do we want to stick to the > existing approach until we have improved the general reset approach? I don't think the current code is really broken, so personally I'd prefer to just leave it alone until we figured out how the reset should work in general. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn 2019-05-28 8:33 ` Cornelia Huck @ 2019-05-28 9:29 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 18+ messages in thread From: Philippe Mathieu-Daudé @ 2019-05-28 9:29 UTC (permalink / raw) To: Cornelia Huck, David Hildenbrand Cc: Thomas Huth, Markus Armbruster, QEMU Developers, Halil Pasic, Christian Borntraeger, qemu-s390x On 5/28/19 10:33 AM, Cornelia Huck wrote: > On Tue, 28 May 2019 10:29:09 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 24.05.19 21:45, Christian Borntraeger wrote: >>> >>> >>> On 24.05.19 21:00, David Hildenbrand wrote: >>>> On 24.05.19 20:36, David Hildenbrand wrote: >>>>> On 24.05.19 20:28, Christian Borntraeger wrote: >>>>>> >>>>>> >>>>>> On 24.05.19 20:04, David Hildenbrand wrote: >>>>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote: >>>>>>>> Hi Christian, >>>>>>>> >>>>>>>> I'm having hard time to understand why the S390_IPL object calls >>>>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while >>>>>>>> being QOM'ified (it has a reset method). >>>>>>>> >>>>>>>> It doesn't seem to have a qdev children added explicitly to it. >>>>>>>> I see it is used as a singleton, what else am I missing? >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Phil. >>>>>>>> >>>>>>> >>>>>>> Looks like I added it back then (~4 years ago) when converting it into a >>>>>>> TYPE_DEVICE. >>>>>>> >>>>>>> I could imagine that - back then - this was needed because only >>>>>>> TYPE_SYS_BUS_DEVICE would recursively get reset. >>>>>> >>>>>> Yes, back then singleton devices were not recursively resetted. Has that changed? >>>>> >>>>> Hacking that call out, I don't see it getting called anymore. So it is >>>>> still required. The question is if it can be reworked. >>>>> >>>> >>>> Yes, as it is not a sysbus device, it won't get reset. >>>> The owner (machine) has to take care of this. The following works: >>>> >>>> >>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>>> index b93750c14e..91a31c2cd0 100644 >>>> --- a/hw/s390x/ipl.c >>>> +++ b/hw/s390x/ipl.c >>>> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) >>>> */ >>>> ipl->compat_start_addr = ipl->start_addr; >>>> ipl->compat_bios_start_addr = ipl->bios_start_addr; >>>> - qemu_register_reset(qdev_reset_all_fn, dev); >>>> error: >>>> error_propagate(errp, err); >>>> } >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index bbc6e8fa0b..658ab529a1 100644 >>>> --- a/hw/s390x/s390-virtio-ccw.c >>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg) >>>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu); >>>> } >>>> >>>> +static void s390_ipl_reset(void) >>>> +{ >>>> + qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL))); >>>> +} >>>> + >>>> static void s390_machine_reset(void) >>>> { >>>> enum s390_reset reset_type; >>>> @@ -353,6 +358,7 @@ static void s390_machine_reset(void) >>>> case S390_RESET_EXTERNAL: >>>> case S390_RESET_REIPL: >>>> qemu_devices_reset(); >>>> + s390_ipl_reset(); >>>> s390_crypto_reset(); >>>> >>>> /* configure and start the ipl CPU only */ >>>> >>> >>> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted, >>> but qom devices not. >>> >> >> Shall I send that as a proper patch, or do we want to stick to the >> existing approach until we have improved the general reset approach? > > I don't think the current code is really broken, so personally I'd > prefer to just leave it alone until we figured out how the reset should > work in general. Agreed, I'd rather wait we better understand QOM/reset limitations, then fix this properly, and finally kill the qdev_reset_all_fn() function. Thanks all for having a look at this btw :) Regards, Phil. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-05-29 10:33 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-24 17:54 [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn Philippe Mathieu-Daudé 2019-05-24 18:04 ` David Hildenbrand 2019-05-24 18:20 ` Philippe Mathieu-Daudé 2019-05-24 18:28 ` Christian Borntraeger 2019-05-24 18:36 ` David Hildenbrand 2019-05-24 19:00 ` David Hildenbrand 2019-05-24 19:45 ` Christian Borntraeger 2019-05-24 19:58 ` David Hildenbrand 2019-05-25 15:03 ` Peter Maydell 2019-05-27 7:52 ` Markus Armbruster 2019-05-27 9:59 ` Philippe Mathieu-Daudé 2019-05-27 18:55 ` Peter Maydell 2019-05-28 5:02 ` Markus Armbruster 2019-05-29 6:08 ` Markus Armbruster 2019-05-29 10:32 ` Philippe Mathieu-Daudé 2019-05-28 8:29 ` David Hildenbrand 2019-05-28 8:33 ` Cornelia Huck 2019-05-28 9:29 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.