Re: [Qemu-block] [PATCH v3 09/33] add doc about Resettable interface

[Damien Hedde]

On 7/31/19 8:30 AM, David Gibson wrote:
> On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote:
>> Signed-off-by: Damien Hedde <address@hidden>
>> ---
>>  docs/devel/reset.txt | 165 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 165 insertions(+)
>>  create mode 100644 docs/devel/reset.txt
>>
>> diff --git a/docs/devel/reset.txt b/docs/devel/reset.txt
>> new file mode 100644
>> index 0000000000..c7a1eb068f
>> --- /dev/null
>> +++ b/docs/devel/reset.txt
>> @@ -0,0 +1,165 @@
>> +
>> +=====
>> +Reset
>> +=====
>> +
>> +The reset of qemu objects is handled using the Resettable interface declared
>> +in *include/hw/resettable.h*.
>> +As of now DeviceClass and BusClass implement this interface.
>> +
>> +
>> +Triggering reset
>> +----------------
>> +
>> +The function *resettable_reset* is used to trigger a reset on a given
>> +object.
>> +void resettable_reset(Object *obj, bool cold)
>> +
>> +The parameter *obj* must implement the Resettable interface.
> 
> And what happens if it doesn't?  This function has no way to report an
> error.

In the function, while retrieving the Resettable class, there is an
assert checking the obj is compatible. We could put an error argument
there to report that if that's preferable.
But then it means an error object should be given for every reset call.

> 
>> +The parameter *cold* is a boolean specifying whether to do a cold or warm
>> +reset
> 
> This doc really needs to explain the distinction between cold and warm
> reset.

ok

> 
>> +For Devices and Buses there is also the corresponding helpers:
>> +void device_reset(Device *dev, bool cold)
>> +void bus_reset(Device *dev, bool cold)
> 
> What's the semantic difference between resetting a bus and resetting
> the bridge device which owns it?

I can't speak for specific cases.
BusClass has already a reset method and qbus_reset_all is used as well
as qdev_reset_all in current code base. Currently both devices and buses
are used as reset entry point. I'm just keeping it that way.

> 
>> +If one wants to put an object into a reset state. There is the
>> +*resettable_assert_reset* function.
>> +void resettable_assert_reset(Object *obj, bool cold)
>> +
>> +One must eventually call the function *resettable_deassert_reset* to end the
>> +reset state:
>> +void resettable_deassert_reset(Object *obj, bool cold)
>> +
>> +Calling *resettable_assert_reset* then *resettable_deassert_reset* is the
>> +same as calling *resettable_reset*.
>> +
>> +It is possible to interleave multiple calls to
>> + - resettable_reset,
>> + - resettable_assert_reset, and
>> + - resettable_deassert_reset.
>> +The only constraint is that *resettable_deassert_reset* must be called once
>> +per *resettable_assert_reset* call so that the object leaves the reset 
>> state.
>> +
>> +Therefore there may be several reset sources/controllers of a given object.
>> +The interface handle everything and the controllers do not need to know
>> +anything about each others. The object will leave reset state only when all
>> +controllers released their reset.
>> +
>> +All theses functions must called while holding the iothread lock.
>> +
>> +
>> +Implementing reset for a Resettable object : Multi-phase reset
>> +--------------------------------------------------------------
>> +
>> +The Resettable uses a multi-phase mechanism to handle some ordering 
>> constraints
>> +when resetting multiple object at the same time. For a given object the 
>> reset
>> +procedure is split into three different phases executed in order:
>> + 1 INIT: This phase should set/reset the state of the Resettable it has 
>> when is
>> +         in reset state. Side-effects to others object is forbidden (such as
>> +         setting IO level).
>> + 2 HOLD: This phase corresponds to the external side-effects due to staying 
>> into
>> +         the reset state.
>> + 3 EXIT: This phase corresponds to leaving the reset state. It have both
>> +         local and external effects.
>> +
>> +*resettable_assert_reset* does the INIT and HOLD phases. While
>> +*resettable_deassert_reset* does the EXIT phase.
>> +
>> +When resetting multiple object at the same time. The interface executes the
>> +given phase of the objects before going to the next phase. This guarantee 
>> that
>> +all INIT phases are done before any HOLD phase and so on.
>> +
>> +There is three methods in the interface so must be implemented in an object.
>> +The methods corresponds to the three phases:
>> +```
>> +typedef void (*ResettableInitPhase)(Object *obj);
>> +typedef void (*ResettableHoldPhase)(Object *obj);
>> +typedef void (*ResettableExitPhase)(Object *obj);
>> +typedef struct ResettableClass {
>> +    InterfaceClass parent_class;
>> +
>> +    struct ResettablePhases {
>> +        ResettableInitPhase init;
>> +        ResettableHoldPhase hold;
>> +        ResettableExitPhase exit;
>> +    } phases;
>> +    [...]
>> +} ResettableClass;
>> +```
>> +
>> +Theses methods should be updated when specializing an object. For this the
>> +helper function *resettable_class_set_parent_reset_phases* can be used to
>> +backup parent methods while changing the specialized ones.
>> +void resettable_class_set_parent_reset_phases(ResettableClass *rc,
>> +                                              ResettableInitPhase init,
>> +                                              ResettableHoldPhase hold,
>> +                                              ResettableExitPhase exit,
>> +
>> +For Devices and Buses, some helper exists to know if a device/bus is under
>> +reset and what type of reset it is:
>> +```
>> +bool device_is_resetting(DeviceState *dev);
>> +bool device_is_reset_cold(DeviceState *dev);
> 
> It's not really clear to me when *_is_reset_cold() would be useful.

Useful only for devices/buses that have different cold/warm reset
behavior. In particular this should be used in the reset init phase.

Damien