Re: [PATCH v5 03/13] hw/core: create Resettable QOM interface

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 18 Oct 2019 17:06:20 +0200
Damien Hedde <damien.hedde@greensocs.com> wrote:

> This commit defines an interface allowing multi-phase reset. This aims
> to solve a problem of the actual single-phase reset (built in
> DeviceClass and BusClass): reset behavior is dependent on the order
> in which reset handlers are called. In particular doing external
> side-effect (like setting an qemu_irq) is problematic because receiving
> object may not be reset yet.
> 
> The Resettable interface divides the reset in 3 well defined phases.
> To reset an object tree, all 1st phases are executed then all 2nd then
> all 3rd. See the comments in include/hw/resettable.h for a more complete
> description. The interface defines 3 phases to let the future
> possibility of holding an object into reset for some time.
> 
> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> following commits to use this interface. A mechanism is provided
> to allow executing a transitional reset handler in place of the 2nd
> phase which is executed in children-then-parent order inside a tree.
> This will allow to transition devices and buses smoothly while
> keeping the exact current qdev/qbus reset behavior for now.
> 
> Documentation will be added in a following commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> In this patch only a single reset type is supported, but the interface
> allows for more to be defined.
> 
> I had some thought about problems which may arise when having multiple
> reset types:
> 
> - reset type propagation. Right now we propagate the same reset type
>   to the children. I don't think it will work that with multiple
>   types.
>   For example, if we add pci_bus_reset type: a pci device will
>   implement the reset type but not its children (they may have
>   nothing to do with pci).
>   This can be solved by changing the child_foreach method rules.
>   We should say that child_foreach may change the type it
>   propagates to its children (on a children by children basis).
>   For example, the pci device may just propagate cold reset type
>   to its children.
>   For this we need to pass the type as parameter to child_foreach()
>   method.
> 
> - are all children concerned ? For a given reset type, some child
>   may not need to be reset. As above we can handle that with
>   child_foreach: an resettable object can propagate the reset only
>   to a partial set of its child.
>   For this we need to know the type when we release the reset,
>   that's why I added it to resettable_release_reset() even if it
>   is unused right now.
>   I've also added an opaque parameter to child_foreach. I think
>   we will need that to handle the change of parent because we
>   will need to test if a child is concerned by a reset type: the
>   opaque will allow to use a test callback and get some result.

What about an optional ->filter() callback? That would be invoked if
existing prior to calling the child_foreach callback and could be used
to exclude children from the reset for this round for all callbacks. Or
have it modify the reset type (like in your pci reset -> cold reset
example above), and completely skip it if the reset type has been
modified to a 'no reset' type?

> 
> - several reset types at the same time. I don't another solution
>   than saying we execute *enter* and *hold* phase for every reset
>   type. *exit* will still be executed once for all at the end.
>   It will be up for each object to cope with it if it handle
>   multiple reset types. For *enter* is trivial, calling it twice
>   in a row is no problem given that it should only reset internal
>   state. For *hold* there may be some complication.
> 
> - Obviously we will need to at least an interface class field to hold
>   the supported reset types by the class. Also the reset state will
>   need some modification.
> ---
>  Makefile.objs           |   1 +
>  hw/core/Makefile.objs   |   1 +
>  hw/core/resettable.c    | 230 ++++++++++++++++++++++++++++++++++++++++
>  hw/core/trace-events    |  17 +++
>  include/hw/resettable.h | 199 ++++++++++++++++++++++++++++++++++
>  5 files changed, 448 insertions(+)
>  create mode 100644 hw/core/resettable.c
>  create mode 100644 include/hw/resettable.h