Re: Resetting non-qdev children in a 3-phase reset device

[Philippe Mathieu-Daudé]

+Markus

On 4/9/21 8:13 PM, Peter Maydell wrote:
> I wanted to convert the hw/misc/mps2-scc.c device from old-style
> to 3-phase reset as a prerequisite for another change to the device,
> but I ran into a problem because currently it has some TYPE_DEVICE
> QOM child objects, the LEDs. Because TYPE_DEVICE don't live in the
> qbus hierarchy, the device resets them manually in its DeviceClass::reset
> method:
> 
>     for (i = 0; i < ARRAY_SIZE(s->led); i++) {
>         device_cold_reset(DEVICE(s->led[i]));
>     }
> 
> This makes converting to 3-phase reset awkward. The obvious "natural"
> approach would be to say "in this device's phase X, invoke phase X
> for these objects", but we have no API for that. (The functions which
> would do it, resettable_phase_enter() etc, are static inside resettable.c.)
> 
> Ignoring the phasing and trying to just call device_cold_reset() in
> the 'enter' phase results in an assertion failure, because we trip
> the assert(!enter_phase_in_progress) in resettable_assert_reset(),
> which doesn't expect us to be triggering a reset inside a reset.
> 
> Ideally one would want to be able to add the LEDs to the list of
> things which are children of this object for purposes of reset
> (so they are iterated as part of the resettable_child_foreach()
> logic and their phases are automatically invoked at the right point).
> But for a subclass of DeviceState that's device_reset_child_foreach()
> and it only iterates any qbus children of this device.
> 
> Any clever ideas?

Not very clever... We could kludge it by calling device_legacy_reset()
instead of device_cold_reset() in mps2_fpgaio_reset()... But that
mean we are going backward with the API.


OK, back to read your previous explanations... and the threads around.
https://www.mail-archive.com/qemu-devel@nongnu.org/msg723312.html
https://www.mail-archive.com/qemu-devel@nongnu.org/msg738242.html

"Note that [qdev/qbus hierarchy] is an entirely separate thing
from the QOM hierarchy of parent-and-child object relationships."

Hmm OK. I guess I'm confused seeing parts are overlapping when they
aren't. So setting the QOM parent relationship helps in having a
correct QOM path and the object is displayed nicely in the qom-tree,
but doesn't bring anything on the qdev side.

So back to qdev.

- TYPE_DEVICE (aka 'qdev') is abstract.
  It inherits TYPE_OBJECT.
  It can provide a bus (aka qbus) to plug things.
  It implements TYPE_RESETTABLE_INTERFACE.

- TYPE_SYS_BUS_DEVICE is also abstract.
  It inherits from TYPE_DEVICE, setting qbus=TYPE_SYSTEM_BUS

(
To confuse more, there is some undocumented API called
'device_listener' in qdev which instead uses sysbus:
void device_listener_register(DeviceListener *listener);
void device_listener_unregister(DeviceListener *listener);
)

Making MachineState inherit TYPE_DEVICE and re-implement the
TYPE_RESETTABLE_INTERFACE doesn't seem going in the right
direction...
If TYPE_MACHINE were qdev, its qbus could be a ResetBus. Again
it feels wrong (over engineering).

> Maybe some mechanism for marking "these things which are my
> QOM children I want to be reset when I am reset (so make them> reset children 
> of me and don't reset them as part of the
> qbus-tree-walking)" would be useful. I do think that in a
> lot of cases we want to be doing something closer to "reset
> along the QOM tree".

Eh here you mention QOM again... Shouldn't it be qdev?

I know the LED is just an example of a broader problem.
I indeed took care to add the QOM parent relation:

(qemu) info qom-tree
/machine (mps2-an385-machine)
  /fpgaio (mps2-fpgaio)
    /mps2-fpgaio[0] (memory-region)
    /userled0 (led)
      /unnamed-gpio-in[0] (irq)
    /userled1 (led)
      /unnamed-gpio-in[0] (irq)
  /scc (mps2-scc)
    /mps2-scc[0] (memory-region)
    /scc-led0 (led)
      /unnamed-gpio-in[0] (irq)
    /scc-led1 (led)
      /unnamed-gpio-in[0] (irq)
    ...

So looking at this qom-tree, the reset tree seems to me
more natural than the sysbus one, but IIRC not many models
set this QOM relationship.
QOM objects aren't enforced to have a relation with a parent,
as opposed as recent changes from Markus to always have a qdev
on a qbus). But even without parent they end in the /unattached
container below /machine, so if the reset were there, the
machine could still iterate over the /unattached children.

> I really do need to spend some time working
> out what the right thing with reset is and how we might get
> from where we are now to there...

Well, finally this QOM-tree reset is appealing.

Sorry if I haven't been very helpful :S Still processing the
problem in background...

Regards,

Phil.