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

[Damien Hedde <damien.hedde@greensocs.com>]

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.

> 
>> + * @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