[v3,09/33] add doc about Resettable interface

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 docs/devel/reset.txt | 165 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 165 insertions(+)
 create mode 100644 docs/devel/reset.txt

On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote:
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  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.

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

> +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?

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

> +bool bus_is_resetting(BusState *bus);
> +bool bus_is_reset_cold(BusState *bus);
> +```
> +
> +
> +Implementing the base Resettable behavior : Re-entrance, Hierarchy and Cold/Warm
> +--------------------------------------------------------------------------------
> +
> +There is five others methods in the interface to handle the base mechanics
> +of the Resettable interface. The methods should be implemented in object
> +base class. DeviceClass and BusClass implement them.
> +
> +```
> +typedef bool (*ResettableSetCold)(Object *obj, bool cold);
> +typedef bool (*ResettableSetHoldNeeded)(Object *obj, bool hold_needed);
> +typedef uint32_t (*ResettableGetCount)(Object *obj);
> +typedef uint32_t (*ResettableIncrementCount)(Object *obj);
> +typedef uint32_t (*ResettableDecrementCount)(Object *obj);
> +typedef void (*ResettableForeachChild)(Object *obj, void (*visitor)(Object *));
> +typedef struct ResettableClass {
> +    InterfaceClass parent_class;
> +
> +    [...]
> +
> +    ResettableSetCold set_cold;
> +    ResettableSetHoldNeeded set_hold_needed;
> +    ResettableGetCount get_count;
> +    ResettableIncrementCount increment_count;
> +    ResettableDecrementCount decrement_count;
> +    ResettableForeachChild foreach_child;
> +} ResettableClass;
> +```
> +
> +*set_cold* is used when entering reset, before calling the init phase, to
> +indicate the reset type.
> +
> +*set_hold_needed* is used to set/clear and retrieve an "hold_needed" flag.
> +This flag allows to omly execute the hold pahse when required.
> +
> +As stated above, several reset procedures can be concurrent on an object.
> +This is handled with the three methods *get_count*, *increment_count* and
> +*decrement_count*. An object is in reset state if the count is non-zero.
> +
> +The reset hierarchy is handled using the *foreach_child* method. This method
> +executes a given function on every reset "child".
> +
> +In DeviceClass and BusClass the base behavior is to mimic the legacy qdev
> +reset. Reset hierarchy follows the qdev/qbus tree.
> +
> +Reset control through GPIO
> +--------------------------
> +
> +For devices, two reset inputs can be added: one for the cold, one the warm
> +reset. This is done using the following function.
> +```
> +typedef enum DeviceResetActiveType {
> +    DEVICE_RESET_ACTIVE_LOW,
> +    DEVICE_RESET_ACTIVE_HIGH,
> +} DeviceResetActiveType;
> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
> +                                   bool cold, DeviceResetActiveType type);
> +```

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 <damien.hedde@greensocs.com>
>> ---
>>  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

On Wed, 31 Jul 2019 at 07:33, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote:
> > +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.

Trying to reset an object that isn't actually resettable would
be a programming error -- I think asserting is a reasonable
response to that.

thanks
-- PMM

On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  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
> +----------------

I think this would be clearer if we documented it the other way around:

  This section documents the APIs which "users" of a resettable
  object should use to control it.

  A resettable object can be put into its "in reset" state and
  held there indefinitely.

  [documentation for resettable_assert/deassert_reset goes here]

  If you want to just trigger a reset event but not leave the
  object in reset for any period of time, you can use
  resettable_reset(), which is a convenience function identical
  in behaviour to calling resettable_assert() and then immediately
  calling resettable_deassert().

  [resettable_reset() docs here]

> +
> +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.
> +The parameter *cold* is a boolean specifying whether to do a cold or warm
> +reset
> +
> +For Devices and Buses there is also the corresponding helpers:
> +void device_reset(Device *dev, bool cold)
> +void bus_reset(Device *dev, bool cold)

We should explain what these do.

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

"handles"

> +anything about each others. The object will leave reset state only when all

"each other"

> +controllers released their reset.

"release"

> +
> +All theses functions must called while holding the iothread lock.

"these"; "be called"

> +
> +
> +Implementing reset for a Resettable object : Multi-phase reset
> +--------------------------------------------------------------

  This section documents the APIs that an implementation of a
  resettable object must provide.

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

"objects"

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

We should be a bit more detailed here. I had some text in
my review of patch 1 for the doc-comments which we can put here too.

> +
> +*resettable_assert_reset* does the INIT and HOLD phases. While
> +*resettable_deassert_reset* does the EXIT phase.

Delete this bit -- we don't want to muddle the reader up between
the 'user' APIs and the 'implementer' APIs.

> +
> +When resetting multiple object at the same time. The interface executes the

Comma, not full stop.

> +given phase of the objects before going to the next phase. This guarantee that

"guarantees"

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

"are"; "that must"

> +The methods corresponds to the three phases:

"correspond"

> +```
> +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

"These"

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

"helpers"

> +reset and what type of reset it is:
> +```
> +bool device_is_resetting(DeviceState *dev);
> +bool device_is_reset_cold(DeviceState *dev);
> +bool bus_is_resetting(BusState *bus);
> +bool bus_is_reset_cold(BusState *bus);
> +```

What are these for? Are they part of the API intended for 'users'
of a resettable device', or something that the 'implementor' might
want to use as part of their implementation. More detail would be good.

> +
> +
> +Implementing the base Resettable behavior : Re-entrance, Hierarchy and Cold/Warm
> +--------------------------------------------------------------------------------

  This section documents parts of the reset mechanism that you only
  need to know about if you are extending it to work with a new
  base class other than DeviceClass or BusClass, or maintaining
  the existing code in those classes. Most people can ignore it.

> +
> +There is five others methods in the interface to handle the base mechanics

"are five other methods"

> +of the Resettable interface. The methods should be implemented in object
> +base class. DeviceClass and BusClass implement them.
> +
> +```
> +typedef bool (*ResettableSetCold)(Object *obj, bool cold);
> +typedef bool (*ResettableSetHoldNeeded)(Object *obj, bool hold_needed);
> +typedef uint32_t (*ResettableGetCount)(Object *obj);
> +typedef uint32_t (*ResettableIncrementCount)(Object *obj);
> +typedef uint32_t (*ResettableDecrementCount)(Object *obj);
> +typedef void (*ResettableForeachChild)(Object *obj, void (*visitor)(Object *));
> +typedef struct ResettableClass {
> +    InterfaceClass parent_class;
> +
> +    [...]
> +
> +    ResettableSetCold set_cold;
> +    ResettableSetHoldNeeded set_hold_needed;
> +    ResettableGetCount get_count;
> +    ResettableIncrementCount increment_count;
> +    ResettableDecrementCount decrement_count;
> +    ResettableForeachChild foreach_child;
> +} ResettableClass;
> +```
> +
> +*set_cold* is used when entering reset, before calling the init phase, to
> +indicate the reset type.
> +
> +*set_hold_needed* is used to set/clear and retrieve an "hold_needed" flag.
> +This flag allows to omly execute the hold pahse when required.
> +
> +As stated above, several reset procedures can be concurrent on an object.
> +This is handled with the three methods *get_count*, *increment_count* and
> +*decrement_count*. An object is in reset state if the count is non-zero.
> +
> +The reset hierarchy is handled using the *foreach_child* method. This method
> +executes a given function on every reset "child".
> +
> +In DeviceClass and BusClass the base behavior is to mimic the legacy qdev
> +reset. Reset hierarchy follows the qdev/qbus tree.

We should document the behaviour of the new system specifically,
without saying it's like the old legacy reset. After all the idea
is that the legacy reset is going to go away, so readers should be
able to understand the new system without first knowing about the
old one.

> +
> +Reset control through GPIO
> +--------------------------
> +
> +For devices, two reset inputs can be added: one for the cold, one the warm
> +reset. This is done using the following function.
> +```
> +typedef enum DeviceResetActiveType {
> +    DEVICE_RESET_ACTIVE_LOW,
> +    DEVICE_RESET_ACTIVE_HIGH,
> +} DeviceResetActiveType;
> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
> +                                   bool cold, DeviceResetActiveType type);
> +```

Rather than just quoting the prototype of the function, we should
provide a more descriptive documentation of what this feature is
for and why you'd want to use it. Then we can just refer the
reader to the function (which should have its own doc comment)
for the exact details of how the API works.

thanks
-- PMM

On Wed, 31 Jul 2019 at 07:33, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote:
> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > +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?

We should definitely explain this in the documentation, but
consider for instance a SCSI controller. Resetting the
SCSI controller puts all its registers back into whatever
the reset state is for the device, as well as resetting
everything on the SCSI bus. Resetting just the SCSI bus
resets the disks and so on on the bus, but doesn't change
the state of the controller itself, which remains programmed
with whatever state the guest has set up.

PCI has a similar distinction between resetting the controller
and resetting the bus.

Note that we have this distinction in the current APIs too:
qbus_reset_all() vs qdev_reset_all().


thanks
-- PMM

On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@greensocs.com> wrote:

> +For Devices and Buses there is also the corresponding helpers:
> +void device_reset(Device *dev, bool cold)
> +void bus_reset(Device *dev, bool cold

Just noticed, but the prototype here is wrong: bus_reset() takes
a BusState*, not a Device*.

thanks
-- PMM

On Wed, Aug 07, 2019 at 11:34:41AM +0100, Peter Maydell wrote:
> On Wed, 31 Jul 2019 at 07:33, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote:
> > > +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.
> 
> Trying to reset an object that isn't actually resettable would
> be a programming error -- I think asserting is a reasonable
> response to that.

Yeah, fair enough.

On Wed, Aug 07, 2019 at 05:01:42PM +0100, Peter Maydell wrote:
> On Wed, 31 Jul 2019 at 07:33, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote:
> > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > > +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?
> 
> We should definitely explain this in the documentation, but
> consider for instance a SCSI controller. Resetting the
> SCSI controller puts all its registers back into whatever
> the reset state is for the device, as well as resetting
> everything on the SCSI bus. Resetting just the SCSI bus
> resets the disks and so on on the bus, but doesn't change
> the state of the controller itself, which remains programmed
> with whatever state the guest has set up.
> 
> PCI has a similar distinction between resetting the controller
> and resetting the bus.
> 
> Note that we have this distinction in the current APIs too:
> qbus_reset_all() vs qdev_reset_all().

Yeah, sorry, I didn't express my concern very well... and now I've
kind of forgotten the details of it.  I think the oddities you also
pointed out with the state saving made me think the two were
sorta equivalent in this patchset, but the interface suggested otherwise.