Re: [PATCH v7 03/11] hw/core: create Resettable QOM interface

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 1/20/20 10:08 AM, Damien Hedde wrote:
> On 1/18/20 7:42 AM, Philippe Mathieu-Daudé wrote:
>> On 1/15/20 1:36 PM, Damien Hedde 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>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>
>>> v7 update: un-nest struct ResettablePhases
>>> ---
>>>    Makefile.objs           |   1 +
>>>    include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>>>    hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>>>    hw/core/Makefile.objs   |   1 +
>>>    hw/core/trace-events    |  17 +++
>>>    5 files changed, 468 insertions(+)
>>>    create mode 100644 include/hw/resettable.h
>>>    create mode 100644 hw/core/resettable.c
>>>
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index 7c1e50f9d6..9752d549b4 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>>>    trace-events-subdirs += net
>>>    trace-events-subdirs += ui
>>>    endif
>>> +trace-events-subdirs += hw/core
>>>    trace-events-subdirs += hw/display
>>>    trace-events-subdirs += qapi
>>>    trace-events-subdirs += qom
>>> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
>>> new file mode 100644
>>> index 0000000000..58b3df4c22
>>> --- /dev/null
>>> +++ b/include/hw/resettable.h
>>> @@ -0,0 +1,211 @@
>>> +/*
>>> + * Resettable interface header.
>>> + *
>>> + * Copyright (c) 2019 GreenSocs SAS
>>> + *
>>> + * Authors:
>>> + *   Damien Hedde
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#ifndef HW_RESETTABLE_H
>>> +#define HW_RESETTABLE_H
>>> +
>>> +#include "qom/object.h"
>>> +
>>> +#define TYPE_RESETTABLE_INTERFACE "resettable"
>>> +
>>> +#define RESETTABLE_CLASS(class) \
>>> +    OBJECT_CLASS_CHECK(ResettableClass, (class),
>>> TYPE_RESETTABLE_INTERFACE)
>>> +
>>> +#define RESETTABLE_GET_CLASS(obj) \
>>> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
>>> +
>>> +typedef struct ResettableState ResettableState;
>>> +
>>> +/**
>>> + * ResetType:
>>> + * Types of reset.
>>> + *
>>> + * + Cold: reset resulting from a power cycle of the object.
>>> + *
>>> + * TODO: Support has to be added to handle more types. In particular,
>>> + * ResettableState structure needs to be expanded.
>>> + */
>>> +typedef enum ResetType {
>>> +    RESET_TYPE_COLD,
>>> +} ResetType;
>>> +
>>> +/*
>>> + * ResettableClass:
>>> + * Interface for resettable objects.
>>> + *
>>> + * See docs/devel/reset.rst for more detailed information about how
>>> QEMU models
>>> + * reset. This whole API must only be used when holding the iothread
>>> mutex.
>>> + *
>>> + * All objects which can be reset must implement this interface;
>>> + * it is usually provided by a base class such as DeviceClass or
>>> BusClass.
>>> + * Every Resettable object must maintain some state tracking the
>>> + * progress of a reset operation by providing a ResettableState
>>> structure.
>>> + * The functions defined in this module take care of updating the
>>> + * state of the reset.
>>> + * The base class implementation of the interface provides this
>>> + * state and implements the associated method: get_state.
>>> + *
>>> + * Concrete object implementations (typically specific devices
>>> + * such as a UART model) should provide the functions
>>> + * for the phases.enter, phases.hold and phases.exit methods, which
>>> + * they can set in their class init function, either directly or
>>> + * by calling resettable_class_set_parent_phases().
>>> + * The phase methods are guaranteed to only only ever be called once
>>> + * for any reset event, in the order 'enter', 'hold', 'exit'.
>>> + * An object will always move quickly from 'enter' to 'hold'
>>> + * but might remain in 'hold' for an arbitrary period of time
>>> + * before eventually reset is deasserted and the 'exit' phase is called.
>>> + * Object implementations should be prepared for functions handling
>>> + * inbound connections from other devices (such as qemu_irq handler
>>> + * functions) to be called at any point during reset after their
>>> + * 'enter' method has been called.
>>> + *
>>> + * Users of a resettable object should not call these methods
>>> + * directly, but instead use the function resettable_reset().
>>> + *
>>> + * @phases.enter: This phase is called when the object enters reset. It
>>> + * should reset local state of the object, but it must not do
>>> anything that
>>> + * has a side-effect on other objects, such as raising or lowering a
>>> qemu_irq
>>> + * line or reading or writing guest memory. It takes the reset's type as
>>> + * argument.
>>> + *
>>> + * @phases.hold: This phase is called for entry into reset, once
>>> every object
>>> + * in the system which is being reset has had its @phases.enter
>>> method called.
>>> + * At this point devices can do actions that affect other objects.
>>> + *
>>> + * @phases.exit: This phase is called when the object leaves the
>>> reset state.
>>> + * Actions affecting other objects are permitted.
>>> + *
>>> + * @get_state: Mandatory method which must return a pointer to a
>>> + * ResettableState.
>>> + *
>>> + * @get_transitional_function: transitional method to handle
>>> Resettable objects
>>> + * not yet fully moved to this interface. It will be removed as soon
>>> as it is
>>> + * not needed anymore. This method is optional and may return a
>>> pointer to a
>>> + * function to be used instead of the phases. If the method exists
>>> and returns
>>> + * a non-NULL function pointer then that function is executed as a
>>> replacement
>>> + * of the 'hold' phase method taking the object as argument. The two
>>> other phase
>>> + * methods are not executed.
>>> + *
>>> + * @child_foreach: Executes a given callback on every Resettable
>>> child. Child
>>> + * in this context means a child in the qbus tree, so the children of
>>> a qbus
>>> + * are the devices on it, and the children of a device are all the
>>> buses it
>>> + * owns. This is not the same as the QOM object hierarchy. The
>>> function takes
>>> + * additional opaque and ResetType arguments which must be passed
>>> unmodified to
>>> + * the callback.
>>> + */
>>> +typedef void (*ResettableEnterPhase)(Object *obj, ResetType type);
>>> +typedef void (*ResettableHoldPhase)(Object *obj);
>>> +typedef void (*ResettableExitPhase)(Object *obj);
>>> +typedef ResettableState * (*ResettableGetState)(Object *obj);
>>> +typedef void (*ResettableTrFunction)(Object *obj);
>>> +typedef ResettableTrFunction (*ResettableGetTrFunction)(Object *obj);
>>> +typedef void (*ResettableChildCallback)(Object *, void *opaque,
>>> +                                        ResetType type);
>>> +typedef void (*ResettableChildForeach)(Object *obj,
>>> +                                       ResettableChildCallback cb,
>>> +                                       void *opaque, ResetType type);
>>> +typedef struct ResettablePhases {
>>> +    ResettableEnterPhase enter;
>>> +    ResettableHoldPhase hold;
>>> +    ResettableExitPhase exit;
>>> +} ResettablePhases;
>>> +typedef struct ResettableClass {
>>> +    InterfaceClass parent_class;
>>> +
>>> +    /* Phase methods */
>>> +    ResettablePhases phases;
>>> +
>>> +    /* State access method */
>>> +    ResettableGetState get_state;
>>> +
>>> +    /* Transitional method for legacy reset compatibility */
>>> +    ResettableGetTrFunction get_transitional_function;
>>> +
>>> +    /* Hierarchy handling method */
>>> +    ResettableChildForeach child_foreach;
>>> +} ResettableClass;
>>> +
>>> +/**
>>> + * ResettableState:
>>> + * Structure holding reset related state. The fields should not be
>>> accessed
>>> + * directly; the definition is here to allow further inclusion into
>>> other
>>> + * objects.
>>> + *
>>> + * @count: Number of reset level the object is into. It is
>>> incremented when
>>> + * the reset operation starts and decremented when it finishes.
>>
>> Maybe you can add this comment and the variable in patch 5/11, that
>> would make patch 5 easier to review.
> 
> The variable is used in this patch so I cannot do that.

Oops you are right o_O ...

>>
>>> + * @hold_phase_pending: flag which indicates that we need to invoke
>>> the 'hold'
>>> + * phase handler for this object.
>>> + * @exit_phase_in_progress: true if we are currently in the exit phase
>>> + */
>>> +struct ResettableState {
>>> +    uint32_t count;
>>
>> Maybe simply 'unsigned'?
>>
> 
> Ok.
> 
> --
> Damien