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

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

[Peter Maydell]

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?

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". 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...

thanks
-- PMM

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.

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

[Peter Maydell]

On Sun, 18 Apr 2021 at 21:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> +Markus
>
> On 4/9/21 8:13 PM, Peter Maydell wrote:
> > 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?

No, I meant QOM, ie the relation you see below in the "info qom-tree"
output:

> 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,

I thought they always got parented into somewhere, even if it
was a catch-all bucket somewhere ?

> 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.

...yes, /unattached is what I was thinking about.

My current half-thought-through view is that where we ought
to try to end up is something like:

 * "real" buses should continue to propagate reset
   (A "real" bus is like PCI, SCSI, and other buses where the real
   hardware has a concept of a "bus reset" or where the power to the
   plugged device comes from the bus so that powercycling the
   controller naturally powercycles the devices. Sysbus is not a
   "real" bus; I haven't checked the others to see if we have any
   other non-real buses)
 * reset should follow the QOM tree for objects not on a "real" bus
   (that is, the qdev "reset this device" function should do
   "iterate through my QOM children and reset those which are not
   on a real bus" as well as its current "reset myself" and "reset
   every qbus I have")
 * instead of reset starting with the sysbus and working along the
   qbus hierarchy, we start by resetting the machine. That should
   include resetting all the QOM children of the machine. Any
   device which has a qbus should reset the qbus as part of its
   reset, but only "real" buses reset their children when reset.

That means that, for instance, if you reset an SoC container object
it will reset all the sub-devices within the SoC and the miscellaneous
bits of glue logic like OR gates it might also own[*]. It also means that
CPU objects should no longer need weird special casing, because they
are part of the QOM hierarchy and get reset that way.

[*] Fun fact: TYPE_OR_IRQ inherits directly from TYPE_DEVICE which
means that pretty much no instances of it ever get reset.

There is of course a massive unsolved problem with this idea, which
is the usual "how do we get there from here" one.

(Eventually I think we might be able to collapse TYPE_SYS_BUS_DEVICE
down into TYPE_DEVICE: there is no particular reason why a TYPE_DEVICE
can have GPIO inputs and outputs but only a TYPE_SYS_BUS_DEVICE can
claim to have MMIO regions and IRQs. "Only sysbus devices get reset"
is a big part of why a lot of devices today are sysbus.)

thanks
-- PMM

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

[Markus Armbruster]

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

> On Sun, 18 Apr 2021 at 21:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> +Markus
>>
>> On 4/9/21 8:13 PM, Peter Maydell wrote:
>> > 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?
>
> No, I meant QOM, ie the relation you see below in the "info qom-tree"
> output:
>
>> 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,
>
> I thought they always got parented into somewhere, even if it
> was a catch-all bucket somewhere ?

If a *device* has no QOM parent at realize time, realize sets the QOM
parent to the /machine/unattached/ orphanage.  In device_set_realized():

    if (value && !dev->realized) {
        ...
        if (!obj->parent) {
            gchar *name = g_strdup_printf("device[%d]", unattached_count++);

            object_property_add_child(container_get(qdev_get_machine(),
                                                    "/unattached"),
                                      name, obj);
            unattached_parent = true;
            g_free(name);
        }

As far as I understand this is a crutch to help us cope with
incompletely QOMified devices.

The crutch does not apply to QOM objects that aren't devices.

>> as opposed as recent changes from Markus to always have a qdev
>> on a qbus).

Most qdevs plug into a qbus, but some don't.

DeviceClass member @bus_type names the kind of bus the device plugs
into.  It's a QOM type name.  Example: for a PCI device, it's
TYPE_PCI_BUS, and the device must be plugged into an instance of a
(subtype of) TYPE_PCI_BUS.

If @bus_type is null, the device does not plug into any qbus.

The qbus a device is plugged into is also called the parent bus.  Not to
be confused with the QOM parent.

>>             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.
>
> ...yes, /unattached is what I was thinking about.
>
> My current half-thought-through view is that where we ought
> to try to end up is something like:
>
>  * "real" buses should continue to propagate reset
>    (A "real" bus is like PCI, SCSI, and other buses where the real
>    hardware has a concept of a "bus reset" or where the power to the
>    plugged device comes from the bus so that powercycling the
>    controller naturally powercycles the devices. Sysbus is not a
>    "real" bus; I haven't checked the others to see if we have any
>    other non-real buses)
>  * reset should follow the QOM tree for objects not on a "real" bus
>    (that is, the qdev "reset this device" function should do
>    "iterate through my QOM children and reset those which are not
>    on a real bus" as well as its current "reset myself" and "reset
>    every qbus I have")
>  * instead of reset starting with the sysbus and working along the
>    qbus hierarchy, we start by resetting the machine. That should
>    include resetting all the QOM children of the machine. Any
>    device which has a qbus should reset the qbus as part of its
>    reset, but only "real" buses reset their children when reset.

Sounds like an approximation of reset wire modelling :)

In a real machine, the reset signal travels along "wires" (in quotes,
because it need not be a dedicated wire, although it commonly is)

We're not modelling these wires explicitly so far.  Instead, we make
assumptions such as "reset flows along the qdev tree", which are close
enough except when they aren't.

What you propose is likely closer to reality than what we have now.

Do I make sense?

> That means that, for instance, if you reset an SoC container object
> it will reset all the sub-devices within the SoC and the miscellaneous
> bits of glue logic like OR gates it might also own[*]. It also means that
> CPU objects should no longer need weird special casing, because they
> are part of the QOM hierarchy and get reset that way.
>
> [*] Fun fact: TYPE_OR_IRQ inherits directly from TYPE_DEVICE which
> means that pretty much no instances of it ever get reset.
>
> There is of course a massive unsolved problem with this idea, which
> is the usual "how do we get there from here" one.
>
> (Eventually I think we might be able to collapse TYPE_SYS_BUS_DEVICE
> down into TYPE_DEVICE: there is no particular reason why a TYPE_DEVICE
> can have GPIO inputs and outputs but only a TYPE_SYS_BUS_DEVICE can
> claim to have MMIO regions and IRQs. "Only sysbus devices get reset"
> is a big part of why a lot of devices today are sysbus.)

Sysbus may habe been a design mistake.  It goes back the qdev design
assumption "every device plugs into exactly one bus, every bus is part
of exactly one device, and the main system bus is the root of this
tree".  The assumption ceased to hold long ago, but we still have
sysbus.

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

[Philippe Mathieu-Daudé]

On 4/22/21 3:21 PM, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Sun, 18 Apr 2021 at 21:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> +Markus
>>>
>>> On 4/9/21 8:13 PM, Peter Maydell wrote:
>>>> 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?
>>
>> No, I meant QOM, ie the relation you see below in the "info qom-tree"
>> output:
>>
>>> 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,
>>
>> I thought they always got parented into somewhere, even if it
>> was a catch-all bucket somewhere ?
> 
> If a *device* has no QOM parent at realize time, realize sets the QOM
> parent to the /machine/unattached/ orphanage.  In device_set_realized():
> 
>     if (value && !dev->realized) {
>         ...
>         if (!obj->parent) {
>             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> 
>             object_property_add_child(container_get(qdev_get_machine(),
>                                                     "/unattached"),
>                                       name, obj);
>             unattached_parent = true;
>             g_free(name);
>         }
> 
> As far as I understand this is a crutch to help us cope with
> incompletely QOMified devices.
> 
> The crutch does not apply to QOM objects that aren't devices.
> 
>>> as opposed as recent changes from Markus to always have a qdev
>>> on a qbus).
> 
> Most qdevs plug into a qbus, but some don't.
> 
> DeviceClass member @bus_type names the kind of bus the device plugs
> into.  It's a QOM type name.  Example: for a PCI device, it's
> TYPE_PCI_BUS, and the device must be plugged into an instance of a
> (subtype of) TYPE_PCI_BUS.
> 
> If @bus_type is null, the device does not plug into any qbus.
> 
> The qbus a device is plugged into is also called the parent bus.  Not to
> be confused with the QOM parent.
> 
>>>             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.
>>
>> ...yes, /unattached is what I was thinking about.
>>
>> My current half-thought-through view is that where we ought
>> to try to end up is something like:
>>
>>  * "real" buses should continue to propagate reset
>>    (A "real" bus is like PCI, SCSI, and other buses where the real
>>    hardware has a concept of a "bus reset" or where the power to the
>>    plugged device comes from the bus so that powercycling the
>>    controller naturally powercycles the devices. Sysbus is not a
>>    "real" bus; I haven't checked the others to see if we have any
>>    other non-real buses)
>>  * reset should follow the QOM tree for objects not on a "real" bus
>>    (that is, the qdev "reset this device" function should do
>>    "iterate through my QOM children and reset those which are not
>>    on a real bus" as well as its current "reset myself" and "reset
>>    every qbus I have")
>>  * instead of reset starting with the sysbus and working along the
>>    qbus hierarchy, we start by resetting the machine. That should
>>    include resetting all the QOM children of the machine. Any
>>    device which has a qbus should reset the qbus as part of its
>>    reset, but only "real" buses reset their children when reset.
> 
> Sounds like an approximation of reset wire modelling :)
> 
> In a real machine, the reset signal travels along "wires" (in quotes,
> because it need not be a dedicated wire, although it commonly is)
> 
> We're not modelling these wires explicitly so far.  Instead, we make
> assumptions such as "reset flows along the qdev tree", which are close
> enough except when they aren't.
> 
> What you propose is likely closer to reality than what we have now.

Then maybe reality is easier to model =)

> Do I make sense?

I guess so. Now I wonder if Peter's approach is doable while still
having "incompletely QOMified devices".

But if we can propagate reset tree via QOM, it is a good excuse
to finish QOM'ifying devices and enforce the API to prohibit non-QOM
ones.

And remove the crutch in device_set_realized().

>> That means that, for instance, if you reset an SoC container object
>> it will reset all the sub-devices within the SoC and the miscellaneous
>> bits of glue logic like OR gates it might also own[*]. It also means that
>> CPU objects should no longer need weird special casing, because they
>> are part of the QOM hierarchy and get reset that way.
>>
>> [*] Fun fact: TYPE_OR_IRQ inherits directly from TYPE_DEVICE which
>> means that pretty much no instances of it ever get reset.
>>
>> There is of course a massive unsolved problem with this idea, which
>> is the usual "how do we get there from here" one.
>>
>> (Eventually I think we might be able to collapse TYPE_SYS_BUS_DEVICE
>> down into TYPE_DEVICE: there is no particular reason why a TYPE_DEVICE
>> can have GPIO inputs and outputs but only a TYPE_SYS_BUS_DEVICE can
>> claim to have MMIO regions and IRQs. "Only sysbus devices get reset"
>> is a big part of why a lot of devices today are sysbus.)
> 
> Sysbus may habe been a design mistake.  It goes back the qdev design
> assumption "every device plugs into exactly one bus, every bus is part
> of exactly one device, and the main system bus is the root of this
> tree".  The assumption ceased to hold long ago, but we still have
> sysbus.

This might explain the undocumented API called
'device_listener' in qdev which instead uses sysbus:

void device_listener_register(DeviceListener *listener);
void device_listener_unregister(DeviceListener *listener);

Thanks,

Phil.

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

[Philippe Mathieu-Daudé]

On 4/22/21 4:20 PM, Philippe Mathieu-Daudé wrote:
> On 4/22/21 3:21 PM, Markus Armbruster wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:

>> Most qdevs plug into a qbus, but some don't.
>>
>> DeviceClass member @bus_type names the kind of bus the device plugs
>> into.  It's a QOM type name.  Example: for a PCI device, it's
>> TYPE_PCI_BUS, and the device must be plugged into an instance of a
>> (subtype of) TYPE_PCI_BUS.
>>
>> If @bus_type is null, the device does not plug into any qbus.
>>
>> The qbus a device is plugged into is also called the parent bus.  Not to
>> be confused with the QOM parent.
>>
>>>>             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.
>>>
>>> ...yes, /unattached is what I was thinking about.
>>>
>>> My current half-thought-through view is that where we ought
>>> to try to end up is something like:
>>>
>>>  * "real" buses should continue to propagate reset
>>>    (A "real" bus is like PCI, SCSI, and other buses where the real
>>>    hardware has a concept of a "bus reset" or where the power to the
>>>    plugged device comes from the bus so that powercycling the
>>>    controller naturally powercycles the devices. Sysbus is not a
>>>    "real" bus; I haven't checked the others to see if we have any
>>>    other non-real buses)
>>>  * reset should follow the QOM tree for objects not on a "real" bus
>>>    (that is, the qdev "reset this device" function should do
>>>    "iterate through my QOM children and reset those which are not
>>>    on a real bus" as well as its current "reset myself" and "reset
>>>    every qbus I have")
>>>  * instead of reset starting with the sysbus and working along the
>>>    qbus hierarchy, we start by resetting the machine. That should
>>>    include resetting all the QOM children of the machine. Any
>>>    device which has a qbus should reset the qbus as part of its
>>>    reset, but only "real" buses reset their children when reset.
>>
>> Sounds like an approximation of reset wire modelling :)
>>
>> In a real machine, the reset signal travels along "wires" (in quotes,
>> because it need not be a dedicated wire, although it commonly is)
>>
>> We're not modelling these wires explicitly so far.  Instead, we make
>> assumptions such as "reset flows along the qdev tree", which are close
>> enough except when they aren't.
>>
>> What you propose is likely closer to reality than what we have now.
> 
> Then maybe reality is easier to model =)
> 
>> Do I make sense?
> 
> I guess so. Now I wonder if Peter's approach is doable while still
> having "incompletely QOMified devices".
> 
> But if we can propagate reset tree via QOM, it is a good excuse
> to finish QOM'ifying devices and enforce the API to prohibit non-QOM
> ones.
> 
> And remove the crutch in device_set_realized().
> 
>>> That means that, for instance, if you reset an SoC container object
>>> it will reset all the sub-devices within the SoC and the miscellaneous
>>> bits of glue logic like OR gates it might also own[*]. It also means that
>>> CPU objects should no longer need weird special casing, because they
>>> are part of the QOM hierarchy and get reset that way.
>>>
>>> [*] Fun fact: TYPE_OR_IRQ inherits directly from TYPE_DEVICE which
>>> means that pretty much no instances of it ever get reset.
>>>
>>> There is of course a massive unsolved problem with this idea, which
>>> is the usual "how do we get there from here" one.
>>>
>>> (Eventually I think we might be able to collapse TYPE_SYS_BUS_DEVICE
>>> down into TYPE_DEVICE: there is no particular reason why a TYPE_DEVICE
>>> can have GPIO inputs and outputs but only a TYPE_SYS_BUS_DEVICE can
>>> claim to have MMIO regions and IRQs. "Only sysbus devices get reset"
>>> is a big part of why a lot of devices today are sysbus.)

Looking at qemu_register_reset() uses I found this commit:

commit 0c7322cfd3fd382c0096c2a9f00775818a878e13
Date:   Mon Jun 29 08:21:10 2015 +0200

 watchdog/diag288: correctly register for system reset requests

 The diag288 watchdog is no sysbus device, therefore it doesn't get
 triggered on resets automatically using dc->reset.

 Let's register the reset handler manually, so we get correctly notified
 again when a system reset was requested. Also reset the watchdog on
 subsystem resets that don't trigger a full system reset.

Why is the reset() handler in DeviceClass and not in SysbusDeviceClass
if "Only sysbus devices get reset"? ...

>>
>> Sysbus may habe been a design mistake.  It goes back the qdev design
>> assumption "every device plugs into exactly one bus, every bus is part
>> of exactly one device, and the main system bus is the root of this
>> tree".  The assumption ceased to hold long ago, but we still have
>> sysbus.

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

[Philippe Mathieu-Daudé]

On 4/24/21 1:06 AM, Philippe Mathieu-Daudé wrote:
> On 4/22/21 4:20 PM, Philippe Mathieu-Daudé wrote:
>> On 4/22/21 3:21 PM, Markus Armbruster wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>>> Most qdevs plug into a qbus, but some don't.
>>>
>>> DeviceClass member @bus_type names the kind of bus the device plugs
>>> into.  It's a QOM type name.  Example: for a PCI device, it's
>>> TYPE_PCI_BUS, and the device must be plugged into an instance of a
>>> (subtype of) TYPE_PCI_BUS.
>>>
>>> If @bus_type is null, the device does not plug into any qbus.
>>>
>>> The qbus a device is plugged into is also called the parent bus.  Not to
>>> be confused with the QOM parent.
>>>
>>>>>             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.
>>>>
>>>> ...yes, /unattached is what I was thinking about.
>>>>
>>>> My current half-thought-through view is that where we ought
>>>> to try to end up is something like:
>>>>
>>>>  * "real" buses should continue to propagate reset
>>>>    (A "real" bus is like PCI, SCSI, and other buses where the real
>>>>    hardware has a concept of a "bus reset" or where the power to the
>>>>    plugged device comes from the bus so that powercycling the
>>>>    controller naturally powercycles the devices. Sysbus is not a
>>>>    "real" bus; I haven't checked the others to see if we have any
>>>>    other non-real buses)
>>>>  * reset should follow the QOM tree for objects not on a "real" bus
>>>>    (that is, the qdev "reset this device" function should do
>>>>    "iterate through my QOM children and reset those which are not
>>>>    on a real bus" as well as its current "reset myself" and "reset
>>>>    every qbus I have")
>>>>  * instead of reset starting with the sysbus and working along the
>>>>    qbus hierarchy, we start by resetting the machine. That should
>>>>    include resetting all the QOM children of the machine. Any
>>>>    device which has a qbus should reset the qbus as part of its
>>>>    reset, but only "real" buses reset their children when reset.
>>>
>>> Sounds like an approximation of reset wire modelling :)
>>>
>>> In a real machine, the reset signal travels along "wires" (in quotes,
>>> because it need not be a dedicated wire, although it commonly is)
>>>
>>> We're not modelling these wires explicitly so far.  Instead, we make
>>> assumptions such as "reset flows along the qdev tree", which are close
>>> enough except when they aren't.
>>>
>>> What you propose is likely closer to reality than what we have now.
>>
>> Then maybe reality is easier to model =)
>>
>>> Do I make sense?
>>
>> I guess so. Now I wonder if Peter's approach is doable while still
>> having "incompletely QOMified devices".
>>
>> But if we can propagate reset tree via QOM, it is a good excuse
>> to finish QOM'ifying devices and enforce the API to prohibit non-QOM
>> ones.
>>
>> And remove the crutch in device_set_realized().
>>
>>>> That means that, for instance, if you reset an SoC container object
>>>> it will reset all the sub-devices within the SoC and the miscellaneous
>>>> bits of glue logic like OR gates it might also own[*]. It also means that
>>>> CPU objects should no longer need weird special casing, because they
>>>> are part of the QOM hierarchy and get reset that way.
>>>>
>>>> [*] Fun fact: TYPE_OR_IRQ inherits directly from TYPE_DEVICE which
>>>> means that pretty much no instances of it ever get reset.
>>>>
>>>> There is of course a massive unsolved problem with this idea, which
>>>> is the usual "how do we get there from here" one.
>>>>
>>>> (Eventually I think we might be able to collapse TYPE_SYS_BUS_DEVICE
>>>> down into TYPE_DEVICE: there is no particular reason why a TYPE_DEVICE
>>>> can have GPIO inputs and outputs but only a TYPE_SYS_BUS_DEVICE can
>>>> claim to have MMIO regions and IRQs. "Only sysbus devices get reset"
>>>> is a big part of why a lot of devices today are sysbus.)
> 
> Looking at qemu_register_reset() uses I found this commit:
> 
> commit 0c7322cfd3fd382c0096c2a9f00775818a878e13
> Date:   Mon Jun 29 08:21:10 2015 +0200
> 
>  watchdog/diag288: correctly register for system reset requests
> 
>  The diag288 watchdog is no sysbus device, therefore it doesn't get
>  triggered on resets automatically using dc->reset.
> 
>  Let's register the reset handler manually, so we get correctly notified
>  again when a system reset was requested. Also reset the watchdog on
>  subsystem resets that don't trigger a full system reset.
> 
> Why is the reset() handler in DeviceClass and not in SysbusDeviceClass
> if "Only sysbus devices get reset"? ...

Ah, probably because the problem is generic to all busses (ISA, ...)
and not just sysbus.

>>> Sysbus may habe been a design mistake.  It goes back the qdev design
>>> assumption "every device plugs into exactly one bus, every bus is part
>>> of exactly one device, and the main system bus is the root of this
>>> tree".  The assumption ceased to hold long ago, but we still have
>>> sysbus.
> 

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

[Markus Armbruster]

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

> On 4/24/21 1:06 AM, Philippe Mathieu-Daudé wrote:
>
>> Looking at qemu_register_reset() uses I found this commit:
>> 
>> commit 0c7322cfd3fd382c0096c2a9f00775818a878e13
>> Date:   Mon Jun 29 08:21:10 2015 +0200
>> 
>>  watchdog/diag288: correctly register for system reset requests
>> 
>>  The diag288 watchdog is no sysbus device, therefore it doesn't get
>>  triggered on resets automatically using dc->reset.
>> 
>>  Let's register the reset handler manually, so we get correctly notified
>>  again when a system reset was requested. Also reset the watchdog on
>>  subsystem resets that don't trigger a full system reset.
>> 
>> Why is the reset() handler in DeviceClass and not in SysbusDeviceClass
>> if "Only sysbus devices get reset"? ...
>
> Ah, probably because the problem is generic to all busses (ISA, ...)
> and not just sysbus.

diag288 is a bus-less device.  Propagating reset from the root of the
qtree to the leaves won't reach it, because the qtree contains only the
devices that plug into a qbus.

>>>> Sysbus may habe been a design mistake.  It goes back the qdev design
>>>> assumption "every device plugs into exactly one bus, every bus is part
>>>> of exactly one device, and the main system bus is the root of this
>>>> tree".  The assumption ceased to hold long ago, but we still have
>>>> sysbus.

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

[Philippe Mathieu-Daudé]

On 4/24/21 7:28 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>
>>> Looking at qemu_register_reset() uses I found this commit:
>>>
>>> commit 0c7322cfd3fd382c0096c2a9f00775818a878e13
>>> Date:   Mon Jun 29 08:21:10 2015 +0200
>>>
>>>  watchdog/diag288: correctly register for system reset requests
>>>
>>>  The diag288 watchdog is no sysbus device, therefore it doesn't get
>>>  triggered on resets automatically using dc->reset.
>>>
>>>  Let's register the reset handler manually, so we get correctly notified
>>>  again when a system reset was requested. Also reset the watchdog on
>>>  subsystem resets that don't trigger a full system reset.
>>>
>>> Why is the reset() handler in DeviceClass and not in SysbusDeviceClass
>>> if "Only sysbus devices get reset"? ...
>>
>> Ah, probably because the problem is generic to all busses (ISA, ...)
>> and not just sysbus.
> 
> diag288 is a bus-less device.  Propagating reset from the root of the
> qtree to the leaves won't reach it, because the qtree contains only the
> devices that plug into a qbus.

I now understand better the diag288 case, but I still don't understand
the TYPE_APIC one. It has no DeviceClass::reset(), its abstract parent
TYPE_APIC_COMMON register apic_reset_common() but being TYPE_DEVICE it
is not on a qbus. It is somehow connected to the X86CPU object, but the
single call to apic_init_reset() is from do_cpu_init() - not a reset
method -.
Is apic_reset_common() dead code? I don't think so because git-blame
report activity:

  2013-11-05 266) static void apic_reset_common(DeviceState *dev)
  2011-10-16 267) {
  2013-11-05 268)     APICCommonState *s = APIC_COMMON(dev);
  2012-02-17 269)     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
  2015-04-07 270)     uint32_t bsp;
  2011-10-16 271)
  2015-04-07 272)     bsp = s->apicbase & MSR_IA32_APICBASE_BSP;
  2015-04-07 273)     s->apicbase = APIC_DEFAULT_ADDRESS | bsp |
MSR_IA32_APICBASE_ENABLE;
  2016-10-19 274)     s->id = s->initial_apic_id;
  2011-10-16 275)
  2017-01-31 276)     apic_reset_irq_delivered();
  2017-01-31 277)
  2012-02-17 278)     s->vapic_paddr = 0;
  2012-02-17 279)     info->vapic_base_update(s);
  2012-02-17 280)
  2013-11-05 281)     apic_init_reset(dev);
  2011-10-16 282) }

Cc'ing APIC maintainers because I'm lost.

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

[Philippe Mathieu-Daudé]

On 4/24/21 3:04 PM, Philippe Mathieu-Daudé wrote:
> On 4/24/21 7:28 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>>
>>>> Looking at qemu_register_reset() uses I found this commit:
>>>>
>>>> commit 0c7322cfd3fd382c0096c2a9f00775818a878e13
>>>> Date:   Mon Jun 29 08:21:10 2015 +0200
>>>>
>>>>  watchdog/diag288: correctly register for system reset requests
>>>>
>>>>  The diag288 watchdog is no sysbus device, therefore it doesn't get
>>>>  triggered on resets automatically using dc->reset.
>>>>
>>>>  Let's register the reset handler manually, so we get correctly notified
>>>>  again when a system reset was requested. Also reset the watchdog on
>>>>  subsystem resets that don't trigger a full system reset.
>>>>
>>>> Why is the reset() handler in DeviceClass and not in SysbusDeviceClass
>>>> if "Only sysbus devices get reset"? ...
>>>
>>> Ah, probably because the problem is generic to all busses (ISA, ...)
>>> and not just sysbus.
>>
>> diag288 is a bus-less device.  Propagating reset from the root of the
>> qtree to the leaves won't reach it, because the qtree contains only the
>> devices that plug into a qbus.
> 
> I now understand better the diag288 case, but I still don't understand
> the TYPE_APIC one. It has no DeviceClass::reset(), its abstract parent
> TYPE_APIC_COMMON register apic_reset_common() but being TYPE_DEVICE it
> is not on a qbus. It is somehow connected to the X86CPU object, but the
> single call to apic_init_reset() is from do_cpu_init() - not a reset
> method -.
> Is apic_reset_common() dead code? I don't think so because git-blame
> report activity:
> 
>   2013-11-05 266) static void apic_reset_common(DeviceState *dev)
>   2011-10-16 267) {
>   2013-11-05 268)     APICCommonState *s = APIC_COMMON(dev);
>   2012-02-17 269)     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
>   2015-04-07 270)     uint32_t bsp;
>   2011-10-16 271)
>   2015-04-07 272)     bsp = s->apicbase & MSR_IA32_APICBASE_BSP;
>   2015-04-07 273)     s->apicbase = APIC_DEFAULT_ADDRESS | bsp |
> MSR_IA32_APICBASE_ENABLE;
>   2016-10-19 274)     s->id = s->initial_apic_id;
>   2011-10-16 275)
>   2017-01-31 276)     apic_reset_irq_delivered();
>   2017-01-31 277)
>   2012-02-17 278)     s->vapic_paddr = 0;
>   2012-02-17 279)     info->vapic_base_update(s);
>   2012-02-17 280)
>   2013-11-05 281)     apic_init_reset(dev);
>   2011-10-16 282) }

Similarly the KVM counterpart:

  2014-12-10 220) static void kvm_apic_reset(APICCommonState *s)
  2011-10-16 221) {
  2014-08-28 222)     /* Not used by KVM, which uses the CPU mp_state
instead.  */
  2014-08-28 223)     s->wait_for_sipi = 0;
  2016-09-12 224)
  2016-10-31 225)     run_on_cpu(CPU(s->cpu), kvm_apic_put,
RUN_ON_CPU_HOST_PTR(s));
  2014-12-10 226) }

> 
> Cc'ing APIC maintainers because I'm lost.
> 

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

[Peter Maydell]

On Sat, 24 Apr 2021 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> I now understand better the diag288 case, but I still don't understand
> the TYPE_APIC one. It has no DeviceClass::reset(), its abstract parent
> TYPE_APIC_COMMON register apic_reset_common() but being TYPE_DEVICE it
> is not on a qbus. It is somehow connected to the X86CPU object, but the
> single call to apic_init_reset() is from do_cpu_init() - not a reset
> method -.

pc_machine_reset() calls device_legacy_reset(cpu->apic_state)
which is to say it invokes the DeviceState::reset method,
which is either kvm_apic_reset or apic_reset_common.

thanks
-- PMM

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

[Markus Armbruster]

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

> On Sat, 24 Apr 2021 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> I now understand better the diag288 case, but I still don't understand
>> the TYPE_APIC one. It has no DeviceClass::reset(), its abstract parent
>> TYPE_APIC_COMMON register apic_reset_common() but being TYPE_DEVICE it
>> is not on a qbus. It is somehow connected to the X86CPU object, but the
>> single call to apic_init_reset() is from do_cpu_init() - not a reset
>> method -.
>
> pc_machine_reset() calls device_legacy_reset(cpu->apic_state)
> which is to say it invokes the DeviceState::reset method,
> which is either kvm_apic_reset or apic_reset_common.

device_legacy_reset() is deprecated:

    /**
     * device_legacy_reset:
     *
     * Reset a single device (by calling the reset method).
     * Note: This function is deprecated and will be removed when it becomes unused.
     * Please use device_cold_reset() now.
     */
    void device_legacy_reset(DeviceState *dev);

Good to know, but how do we get from here to there?  If we could simply
replace one call by the other, surely we'd have done it already.

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

[Peter Maydell]

On Mon, 26 Apr 2021 at 06:19, Markus Armbruster <armbru@redhat.com> wrote:
> device_legacy_reset() is deprecated:
>
>     /**
>      * device_legacy_reset:
>      *
>      * Reset a single device (by calling the reset method).
>      * Note: This function is deprecated and will be removed when it becomes unused.
>      * Please use device_cold_reset() now.
>      */
>     void device_legacy_reset(DeviceState *dev);
>
> Good to know, but how do we get from here to there?  If we could simply
> replace one call by the other, surely we'd have done it already.

device_legacy_reset() semantics:
 * call the reset method on this device, and do nothing else
device_cold_reset() semantics:
 * call the reset method on this device
 * reset any qbuses this device owns (and so on recursively
   down the qbus tree)

So if the device has no qbuses they're equivalent and we can
just replace one call with the other. If the device does have
a qbus then it's more complicated (and I would start by asking
"do we really want to simulate power-cycling the device but *not*
power-cycling of its bus here?"...)

There are less than 20 callers of device_legacy_reset(), I guess
we should at least go through and identify "which of these are
definitely not resetting a device with qbuses" and update those.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 4/25/21 8:33 PM, Peter Maydell wrote:
> On Sat, 24 Apr 2021 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> I now understand better the diag288 case, but I still don't understand
>> the TYPE_APIC one. It has no DeviceClass::reset(), its abstract parent
>> TYPE_APIC_COMMON register apic_reset_common() but being TYPE_DEVICE it
>> is not on a qbus. It is somehow connected to the X86CPU object, but the
>> single call to apic_init_reset() is from do_cpu_init() - not a reset
>> method -.
> 
> pc_machine_reset() calls device_legacy_reset(cpu->apic_state)
> which is to say it invokes the DeviceState::reset method,
> which is either kvm_apic_reset or apic_reset_common.

Oh, thanks! I guess "convoluted" is the proper adjective to describe
this reset logic. I suppose APIC is a very old device, part of the
Frankenstein PC, so hard to rework (because we are scared of the
implications of changing old & heavily used devices).

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

[Peter Maydell]

On Mon, 26 Apr 2021 at 10:23, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 4/25/21 8:33 PM, Peter Maydell wrote:
> > On Sat, 24 Apr 2021 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >> I now understand better the diag288 case, but I still don't understand
> >> the TYPE_APIC one. It has no DeviceClass::reset(), its abstract parent
> >> TYPE_APIC_COMMON register apic_reset_common() but being TYPE_DEVICE it
> >> is not on a qbus. It is somehow connected to the X86CPU object, but the
> >> single call to apic_init_reset() is from do_cpu_init() - not a reset
> >> method -.
> >
> > pc_machine_reset() calls device_legacy_reset(cpu->apic_state)
> > which is to say it invokes the DeviceState::reset method,
> > which is either kvm_apic_reset or apic_reset_common.
>
> Oh, thanks! I guess "convoluted" is the proper adjective to describe
> this reset logic. I suppose APIC is a very old device, part of the
> Frankenstein PC, so hard to rework (because we are scared of the
> implications of changing old & heavily used devices).

This is mostly just another instance of "our reset logic doesn't
deal well with devices which aren't on buses". The APIC isn't on a bus,
so the machine that uses it has a local workaround to manually arrange
for it to reset, just as it does for the CPU.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 4/26/21 11:33 AM, Peter Maydell wrote:
> On Mon, 26 Apr 2021 at 10:23, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 4/25/21 8:33 PM, Peter Maydell wrote:
>>> On Sat, 24 Apr 2021 at 14:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> I now understand better the diag288 case, but I still don't understand
>>>> the TYPE_APIC one. It has no DeviceClass::reset(), its abstract parent
>>>> TYPE_APIC_COMMON register apic_reset_common() but being TYPE_DEVICE it
>>>> is not on a qbus. It is somehow connected to the X86CPU object, but the
>>>> single call to apic_init_reset() is from do_cpu_init() - not a reset
>>>> method -.
>>>
>>> pc_machine_reset() calls device_legacy_reset(cpu->apic_state)
>>> which is to say it invokes the DeviceState::reset method,
>>> which is either kvm_apic_reset or apic_reset_common.
>>
>> Oh, thanks! I guess "convoluted" is the proper adjective to describe
>> this reset logic. I suppose APIC is a very old device, part of the
>> Frankenstein PC, so hard to rework (because we are scared of the
>> implications of changing old & heavily used devices).
> 
> This is mostly just another instance of "our reset logic doesn't
> deal well with devices which aren't on buses". The APIC isn't on a bus,
> so the machine that uses it has a local workaround to manually arrange
> for it to reset, just as it does for the CPU.

But the APIC is created within the CPU realize():

static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
{
    APICCommonState *apic;
    ObjectClass *apic_class = OBJECT_CLASS(apic_get_class());

    cpu->apic_state = DEVICE(object_new_with_class(apic_class));
    ...

... so I'd expect the CPU (TYPE_X86_CPU) to reset the APIC in its
reset(), not the machine (regardless how the CPU itself is reset).

*But* there is an undocumented MachineClass::wakeup() handler which
complicates the picture:

static void pc_machine_reset(MachineState *machine)
{
    CPUState *cs;
    X86CPU *cpu;

    qemu_devices_reset();

    /* Reset APIC after devices have been reset to cancel
     * any changes that qemu_devices_reset() might have done.
     */
    CPU_FOREACH(cs) {
        cpu = X86_CPU(cs);

        if (cpu->apic_state) {
            device_legacy_reset(cpu->apic_state);
        }
    }
}

static void pc_machine_wakeup(MachineState *machine)
{
    cpu_synchronize_all_states();
    pc_machine_reset(machine);
    cpu_synchronize_all_post_reset();
}

It is called once:

/*
 * Wake the VM after suspend.
 */
static void qemu_system_wakeup(void)
{
    MachineClass *mc;

    mc = current_machine ? MACHINE_GET_CLASS(current_machine) : NULL;

    if (mc && mc->wakeup) {
        mc->wakeup(current_machine);
    }
}

and TYPE_PC_MACHINE is the single object implementing it.

This wakeup is handled in main_loop_should_exit() after the
qemu_reset_requested() check, and pc_machine_wakeup calls
cpu_synchronize_all_post_reset().

Could a 3-phase CPU reset simplify all this?

I agree it is 'mostly just another instance of "our reset logic
doesn't deal well with devices which aren't on buses"', but a
very convoluted one IMHO.

Regards,

Phil.