| Message ID | 20190729145654.14644-2-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multi-phase reset mechanism | expand |
On Mon, 29 Jul 2019 16:56:22 +0200 Damien Hedde <damien.hedde@greensocs.com> wrote: (...) > +/* > + * ResettableClass: > + * Interface for resettable objects. > + * > + * The reset operation is divided in several phases each represented by a > + * method. > + * > + * Each Ressetable must maintain a reset counter in its state, 3 methods allows > + * to interact with it. > + * > + * @phases.init: should reset local state only. Takes a bool @cold argument > + * specifying whether the reset is cold or warm. It must not do side-effect > + * on others objects. I'm having a hard time figuring out what a 'cold' or a 'warm' reset is supposed to be... can you add a definition/guideline somewhere? > + * > + * @phases.hold: side-effects action on others objects due to staying in a > + * resetting state. > + * > + * @phases.exit: leave the reset state, may do side-effects action on others > + * objects. > + * > + * @set_cold: Set whether the current reset is cold or warm. Return the > + * previous flag value. Return value has no meaning if @get_count returns > + * a zero value. Same here. > + * > + * @set_hold_needed: Set hold_needed flag. Return the previous flag value. > + * > + * @get_count: Get the current reset count > + * @increment_count: Increment the reset count, returns the new count > + * @decrement_count: decrement the reset count, returns the new count > + * > + * @foreach_child: Executes a given function on every Resettable child. > + * A child is not a QOM child, but a child a reset meaning. > + */ (...)
On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: > > On Mon, 29 Jul 2019 16:56:22 +0200 > Damien Hedde <damien.hedde@greensocs.com> wrote: > > (...) > > > +/* > > + * ResettableClass: > > + * Interface for resettable objects. > > + * > > + * The reset operation is divided in several phases each represented by a > > + * method. > > + * > > + * Each Ressetable must maintain a reset counter in its state, 3 methods allows > > + * to interact with it. > > + * > > + * @phases.init: should reset local state only. Takes a bool @cold argument > > + * specifying whether the reset is cold or warm. It must not do side-effect > > + * on others objects. > > I'm having a hard time figuring out what a 'cold' or a 'warm' reset is > supposed to be... can you add a definition/guideline somewhere? Generally "cold" reset is "power on" and "warm" is "we were already powered-on, but somebody flipped a reset line somewhere". thanks -- PMM
On Tue, 30 Jul 2019 14:44:21 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: > > > > On Mon, 29 Jul 2019 16:56:22 +0200 > > Damien Hedde <damien.hedde@greensocs.com> wrote: > > > > (...) > > > > > +/* > > > + * ResettableClass: > > > + * Interface for resettable objects. > > > + * > > > + * The reset operation is divided in several phases each represented by a > > > + * method. > > > + * > > > + * Each Ressetable must maintain a reset counter in its state, 3 methods allows > > > + * to interact with it. > > > + * > > > + * @phases.init: should reset local state only. Takes a bool @cold argument > > > + * specifying whether the reset is cold or warm. It must not do side-effect > > > + * on others objects. > > > > I'm having a hard time figuring out what a 'cold' or a 'warm' reset is > > supposed to be... can you add a definition/guideline somewhere? > > Generally "cold" reset is "power on" and "warm" is "we were already > powered-on, but somebody flipped a reset line somewhere". Ok, that makes sense... my main concern is to distinguish that in a generic way, as it is a generic interface. What about adding something like: "A 'cold' reset means that the object to be reset is initially reset; a 'warm' reset means that the object to be reset has already been initialized." Or is that again too generic?
On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: > > On Tue, 30 Jul 2019 14:44:21 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: > > > On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: > > > I'm having a hard time figuring out what a 'cold' or a 'warm' reset is > > > supposed to be... can you add a definition/guideline somewhere? > > > > Generally "cold" reset is "power on" and "warm" is "we were already > > powered-on, but somebody flipped a reset line somewhere". > > Ok, that makes sense... my main concern is to distinguish that in a > generic way, as it is a generic interface. What about adding something > like: > > "A 'cold' reset means that the object to be reset is initially reset; a 'warm' > reset means that the object to be reset has already been initialized." > > Or is that again too generic? I think it doesn't quite capture the idea -- an object can have already been reset and then get a 'cold' reset: this is like having a powered-on machine and then power-cycling it. The 'warm' reset is the vaguer one, because the specific behaviour is somewhat device-dependent (many devices might not have any difference from 'cold' reset, for those that do the exact detail of what doesn't get reset on warm-reset will vary). But every device should have some kind of "as if you power-cycled it" (or for QEMU, "go back to the same state as if you just started QEMU on the command line"). Our current "reset" method is really cold-reset. thanks -- PMM
On 7/30/19 3:59 PM, Peter Maydell wrote: > On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: >> >> On Tue, 30 Jul 2019 14:44:21 +0100 >> Peter Maydell <peter.maydell@linaro.org> wrote: >> >>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: >>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is >>>> supposed to be... can you add a definition/guideline somewhere? >>> >>> Generally "cold" reset is "power on" and "warm" is "we were already >>> powered-on, but somebody flipped a reset line somewhere". >> >> Ok, that makes sense... my main concern is to distinguish that in a >> generic way, as it is a generic interface. What about adding something >> like: >> >> "A 'cold' reset means that the object to be reset is initially reset; a 'warm' >> reset means that the object to be reset has already been initialized." >> >> Or is that again too generic? > > I think it doesn't quite capture the idea -- an object can have already > been reset and then get a 'cold' reset: this is like having a powered-on > machine and then power-cycling it. > > The 'warm' reset is the vaguer one, because the specific behaviour > is somewhat device-dependent (many devices might not have any > difference from 'cold' reset, for those that do the exact detail > of what doesn't get reset on warm-reset will vary). But every > device should have some kind of "as if you power-cycled it" (or > for QEMU, "go back to the same state as if you just started QEMU on the > command line"). Our current "reset" method is really cold-reset. > Exactly. In the following patches, I've tried to replace existing reset calls by cold or warm reset depending on whether: + it is called through the main system reset -> cold + it is called during normal life-time -> warm But I definitely can add some docs/comments to better explain that. -- Damien
On Tue, 30 Jul 2019 16:08:59 +0200 Damien Hedde <damien.hedde@greensocs.com> wrote: > On 7/30/19 3:59 PM, Peter Maydell wrote: > > On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: > >> > >> On Tue, 30 Jul 2019 14:44:21 +0100 > >> Peter Maydell <peter.maydell@linaro.org> wrote: > >> > >>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: > >>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is > >>>> supposed to be... can you add a definition/guideline somewhere? > >>> > >>> Generally "cold" reset is "power on" and "warm" is "we were already > >>> powered-on, but somebody flipped a reset line somewhere". > >> > >> Ok, that makes sense... my main concern is to distinguish that in a > >> generic way, as it is a generic interface. What about adding something > >> like: > >> > >> "A 'cold' reset means that the object to be reset is initially reset; a 'warm' > >> reset means that the object to be reset has already been initialized." > >> > >> Or is that again too generic? > > > > I think it doesn't quite capture the idea -- an object can have already > > been reset and then get a 'cold' reset: this is like having a powered-on > > machine and then power-cycling it. > > > > The 'warm' reset is the vaguer one, because the specific behaviour > > is somewhat device-dependent (many devices might not have any > > difference from 'cold' reset, for those that do the exact detail > > of what doesn't get reset on warm-reset will vary). But every > > device should have some kind of "as if you power-cycled it" (or > > for QEMU, "go back to the same state as if you just started QEMU on the > > command line"). Our current "reset" method is really cold-reset. Ah ok, that makes sense. > > > > Exactly. In the following patches, I've tried to replace existing reset > calls by cold or warm reset depending on whether: > + it is called through the main system reset -> cold > + it is called during normal life-time -> warm > > But I definitely can add some docs/comments to better explain that. Yes, that would be great; I think I now understand enough for looking at the patches.
On Tue, Jul 30, 2019 at 04:08:59PM +0200, Damien Hedde wrote: > > On 7/30/19 3:59 PM, Peter Maydell wrote: > > On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: > >> > >> On Tue, 30 Jul 2019 14:44:21 +0100 > >> Peter Maydell <peter.maydell@linaro.org> wrote: > >> > >>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: > >>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is > >>>> supposed to be... can you add a definition/guideline somewhere? > >>> > >>> Generally "cold" reset is "power on" and "warm" is "we were already > >>> powered-on, but somebody flipped a reset line somewhere". > >> > >> Ok, that makes sense... my main concern is to distinguish that in a > >> generic way, as it is a generic interface. What about adding something > >> like: > >> > >> "A 'cold' reset means that the object to be reset is initially reset; a 'warm' > >> reset means that the object to be reset has already been initialized." > >> > >> Or is that again too generic? > > > > I think it doesn't quite capture the idea -- an object can have already > > been reset and then get a 'cold' reset: this is like having a powered-on > > machine and then power-cycling it. > > > > The 'warm' reset is the vaguer one, because the specific behaviour > > is somewhat device-dependent (many devices might not have any > > difference from 'cold' reset, for those that do the exact detail > > of what doesn't get reset on warm-reset will vary). But every > > device should have some kind of "as if you power-cycled it" (or > > for QEMU, "go back to the same state as if you just started QEMU on the > > command line"). Our current "reset" method is really cold-reset. > > > > Exactly. In the following patches, I've tried to replace existing reset > calls by cold or warm reset depending on whether: > + it is called through the main system reset -> cold > + it is called during normal life-time -> warm > > But I definitely can add some docs/comments to better explain that. Hrm, that helps, but it still seems pretty vague to me. It's not really my call, but building the concept of warm versus cold resets into such a generic interface seems pretty dubios to me. While it's moderately common for things to have a notion of warm versus cold reset it's certainly not universal. There are many devices where there's no meaningful difference between the two. There are some devices where there are > 2 different types of reset suitable for various different situations. Is there any way this could work with it usually just presenting the cold reset (which is the closest to a universal concept), and any warm or additional resets are handled buy a different instance of the Resettable interface?
Peter Maydell writes: > On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: >> >> On Tue, 30 Jul 2019 14:44:21 +0100 >> Peter Maydell <peter.maydell@linaro.org> wrote: >> >> > On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: >> > > I'm having a hard time figuring out what a 'cold' or a 'warm' reset is >> > > supposed to be... can you add a definition/guideline somewhere? >> > >> > Generally "cold" reset is "power on" and "warm" is "we were already >> > powered-on, but somebody flipped a reset line somewhere". >> >> Ok, that makes sense... my main concern is to distinguish that in a >> generic way, as it is a generic interface. What about adding something >> like: >> >> "A 'cold' reset means that the object to be reset is initially reset; a 'warm' >> reset means that the object to be reset has already been initialized." >> >> Or is that again too generic? > > I think it doesn't quite capture the idea -- an object can have already > been reset and then get a 'cold' reset: this is like having a powered-on > machine and then power-cycling it. > > The 'warm' reset is the vaguer one, because the specific behaviour > is somewhat device-dependent (many devices might not have any > difference from 'cold' reset, for those that do the exact detail > of what doesn't get reset on warm-reset will vary). But every > device should have some kind of "as if you power-cycled it" (or > for QEMU, "go back to the same state as if you just started QEMU on the > command line"). Our current "reset" method is really cold-reset. Is there any concept of locality associated with warm reset? For example, you'd expect a cold reset to happen on the whole system, but I guess a warm reset could be restricted to a single bus. The documentation should give examples of how warm reset could be triggered, and what it could do differently from cold reset. > > thanks > -- PMM -- Cheers, Christophe de Dinechin (IRC c3d)
On 7/31/19 12:17 PM, Christophe de Dinechin wrote: > > Peter Maydell writes: > >> On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: >>> >>> On Tue, 30 Jul 2019 14:44:21 +0100 >>> Peter Maydell <peter.maydell@linaro.org> wrote: >>> >>>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: >>>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is >>>>> supposed to be... can you add a definition/guideline somewhere? >>>> >>>> Generally "cold" reset is "power on" and "warm" is "we were already >>>> powered-on, but somebody flipped a reset line somewhere". >>> >>> Ok, that makes sense... my main concern is to distinguish that in a >>> generic way, as it is a generic interface. What about adding something >>> like: >>> >>> "A 'cold' reset means that the object to be reset is initially reset; a 'warm' >>> reset means that the object to be reset has already been initialized." >>> >>> Or is that again too generic? >> >> I think it doesn't quite capture the idea -- an object can have already >> been reset and then get a 'cold' reset: this is like having a powered-on >> machine and then power-cycling it. >> >> The 'warm' reset is the vaguer one, because the specific behaviour >> is somewhat device-dependent (many devices might not have any >> difference from 'cold' reset, for those that do the exact detail >> of what doesn't get reset on warm-reset will vary). But every >> device should have some kind of "as if you power-cycled it" (or >> for QEMU, "go back to the same state as if you just started QEMU on the >> command line"). Our current "reset" method is really cold-reset. > > Is there any concept of locality associated with warm reset? > For example, you'd expect a cold reset to happen on the whole system, > but I guess a warm reset could be restricted to a single bus. > > The documentation should give examples of how warm reset could be > triggered, and what it could do differently from cold reset. > Not sure we really have some locality difference between cold/warm resets. I think, most generally, locality of reset is on a per-device scale with different grouping level (up to the whole system). But buses are not always the way things are grouped. Inside a soc (and microcontrollers in general) it follows more the clock tree than the internal bus tree. For example on the the machine I worked here (xilinx-zynq-a9) and, you can control by software the reset of basically every single device and the bus too (but resetting the bus does not reset devices on it). On the other hand, there is some buses, like pci, which explicitly defines some bus reset mechanism with differences between cold and warm (some registers keep their values). (see https://wiki.qemu.org/Features/ResetAPI) Regarding cold reset, if a soc supports some power gating, you'll probably have non-global cold reset in the process. Do you mean "real world" example ?
> On 1 Aug 2019, at 11:19, Damien Hedde <damien.hedde@greensocs.com> wrote: > > > On 7/31/19 12:17 PM, Christophe de Dinechin wrote: >> >> Peter Maydell writes: >> >>> On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: >>>> >>>> On Tue, 30 Jul 2019 14:44:21 +0100 >>>> Peter Maydell <peter.maydell@linaro.org> wrote: >>>> >>>>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: >>>>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is >>>>>> supposed to be... can you add a definition/guideline somewhere? >>>>> >>>>> Generally "cold" reset is "power on" and "warm" is "we were already >>>>> powered-on, but somebody flipped a reset line somewhere". >>>> >>>> Ok, that makes sense... my main concern is to distinguish that in a >>>> generic way, as it is a generic interface. What about adding something >>>> like: >>>> >>>> "A 'cold' reset means that the object to be reset is initially reset; a 'warm' >>>> reset means that the object to be reset has already been initialized." >>>> >>>> Or is that again too generic? >>> >>> I think it doesn't quite capture the idea -- an object can have already >>> been reset and then get a 'cold' reset: this is like having a powered-on >>> machine and then power-cycling it. >>> >>> The 'warm' reset is the vaguer one, because the specific behaviour >>> is somewhat device-dependent (many devices might not have any >>> difference from 'cold' reset, for those that do the exact detail >>> of what doesn't get reset on warm-reset will vary). But every >>> device should have some kind of "as if you power-cycled it" (or >>> for QEMU, "go back to the same state as if you just started QEMU on the >>> command line"). Our current "reset" method is really cold-reset. >> >> Is there any concept of locality associated with warm reset? >> For example, you'd expect a cold reset to happen on the whole system, >> but I guess a warm reset could be restricted to a single bus. >> >> The documentation should give examples of how warm reset could be >> triggered, and what it could do differently from cold reset. >> > > Not sure we really have some locality difference between cold/warm > resets. I think, most generally, locality of reset is on a per-device > scale with different grouping level (up to the whole system). But buses > are not always the way things are grouped. > > Inside a soc (and microcontrollers in general) it follows more the clock > tree than the internal bus tree. For example on the the machine I worked > here (xilinx-zynq-a9) and, you can control by software the reset of > basically every single device and the bus too (but resetting the bus > does not reset devices on it). > > On the other hand, there is some buses, like pci, which explicitly > defines some bus reset mechanism with differences between cold and warm > (some registers keep their values). (see > https://wiki.qemu.org/Features/ResetAPI) > > Regarding cold reset, if a soc supports some power gating, you'll > probably have non-global cold reset in the process. > > Do you mean "real world" example ? I think that a variant of what you just wrote would be fine. I was not aware for example that you could have non-global cold reset. Thanks Christophe
On 7/31/19 7:46 AM, David Gibson wrote: > On Tue, Jul 30, 2019 at 04:08:59PM +0200, Damien Hedde wrote: >> >> On 7/30/19 3:59 PM, Peter Maydell wrote: >>> On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: >>>> >>>> On Tue, 30 Jul 2019 14:44:21 +0100 >>>> Peter Maydell <peter.maydell@linaro.org> wrote: >>>> >>>>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: >>>>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is >>>>>> supposed to be... can you add a definition/guideline somewhere? >>>>> >>>>> Generally "cold" reset is "power on" and "warm" is "we were already >>>>> powered-on, but somebody flipped a reset line somewhere". >>>> >>>> Ok, that makes sense... my main concern is to distinguish that in a >>>> generic way, as it is a generic interface. What about adding something >>>> like: >>>> >>>> "A 'cold' reset means that the object to be reset is initially reset; a 'warm' >>>> reset means that the object to be reset has already been initialized." >>>> >>>> Or is that again too generic? >>> >>> I think it doesn't quite capture the idea -- an object can have already >>> been reset and then get a 'cold' reset: this is like having a powered-on >>> machine and then power-cycling it. >>> >>> The 'warm' reset is the vaguer one, because the specific behaviour >>> is somewhat device-dependent (many devices might not have any >>> difference from 'cold' reset, for those that do the exact detail >>> of what doesn't get reset on warm-reset will vary). But every >>> device should have some kind of "as if you power-cycled it" (or >>> for QEMU, "go back to the same state as if you just started QEMU on the >>> command line"). Our current "reset" method is really cold-reset. >>> >> >> Exactly. In the following patches, I've tried to replace existing reset >> calls by cold or warm reset depending on whether: >> + it is called through the main system reset -> cold >> + it is called during normal life-time -> warm >> >> But I definitely can add some docs/comments to better explain that. > > Hrm, that helps, but it still seems pretty vague to me. > > It's not really my call, but building the concept of warm versus cold > resets into such a generic interface seems pretty dubios to me. While > it's moderately common for things to have a notion of warm versus cold > reset it's certainly not universal. There are many devices where > there's no meaningful difference between the two. There are some > devices where there are > 2 different types of reset suitable for > various different situations. > > Is there any way this could work with it usually just presenting the > cold reset (which is the closest to a universal concept), and any warm > or additional resets are handled buy a different instance of the > Resettable interface? > In my current implementation, cold/warm is only a question of setting a flag before calling the reset methods. I rely and the reset methods to check that flag if necessary. The feature can be added/removed without pain (if we stick with device_reset to do a cold one). So if it's better, I can put it aside for now. IMO handling warm reset with another interface is probably not a good idea because it will need another load of methods. -- Damien
On Mon, 29 Jul 2019 at 15:58, Damien Hedde <damien.hedde@greensocs.com> wrote: > > This commit defines an interface allowing multi-phase reset. > The phases are INIT, HOLD and EXIT. Each phase has an associated method > in the class. > > The reset of a Resettable is controlled with 2 functions: > - resettable_assert_reset which starts the reset operation. > - resettable_deassert_reset which ends the reset operation. > > There is also a `resettable_reset` helper function which does assert then > deassert allowing to do a proper reset in one call. > > In addition, two functions, *resettable_reset_cold_fn* and > *resettable_reset_warm_fn*, are defined. They take only an opaque argument > (for the Resettable object) and can be used as callbacks. > > The Resettable interface is "reentrant", _assert_ can be called several > times and _deasert_ must be called the same number of times so that the deassert > Resettable leave reset state. It is supported by keeping a counter of > the current number of _assert_ calls. The counter maintainance is done > though 3 methods get/increment/decrement_count. A method set_cold is used > to set the cold flag of the reset. Another method set_host_needed is used > to ensure hold phase is executed only if required. > > Reset hierarchy is also supported. Each Resettable may have > sub-Resettable objects. When resetting a Resettable, it is propagated to > its children using the foreach_child method. > > When entering reset, init phases are executed parent-to-child order then > hold phases are executed child-parent order. Why do we execute the hold phase in child-to-parent order? I would have expected this to follow the same order as the init phase. > When leaving reset, exit phases are executed in child-to-parent order. > This will allow to replace current qdev_reset mechanism by this interface > without side-effects on reset order. It makes sense that the exit phase is child-to-parent. > Note: I used an uint32 for the count. This match the type already used > in the existing resetting counter in hw/scsi/vmw_pvscsi.c for the > PVSCSIState. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > --- > Makefile.objs | 1 + > hw/core/Makefile.objs | 1 + > hw/core/resettable.c | 220 ++++++++++++++++++++++++++++++++++++++++ > hw/core/trace-events | 39 +++++++ > include/hw/resettable.h | 126 +++++++++++++++++++++++ > 5 files changed, 387 insertions(+) > create mode 100644 hw/core/resettable.c > create mode 100644 hw/core/trace-events > create mode 100644 include/hw/resettable.h > > diff --git a/Makefile.objs b/Makefile.objs > index 6a143dcd57..a723a47e14 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/hw/core/Makefile.objs b/hw/core/Makefile.objs > index f8481d959f..d9234aa98a 100644 > --- a/hw/core/Makefile.objs > +++ b/hw/core/Makefile.objs > @@ -1,6 +1,7 @@ > # core qdev-related obj files, also used by *-user: > common-obj-y += qdev.o qdev-properties.o > common-obj-y += bus.o reset.o > +common-obj-y += resettable.o > common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o > common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o > # irq.o needed for qdev GPIO handling: > diff --git a/hw/core/resettable.c b/hw/core/resettable.c > new file mode 100644 > index 0000000000..d7d631ce8b > --- /dev/null > +++ b/hw/core/resettable.c > @@ -0,0 +1,220 @@ > +/* > + * Resettable interface. > + * > + * 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. > + */ > + > +#include "qemu/osdep.h" > +#include "qemu/module.h" > +#include "hw/resettable.h" > +#include "trace.h" > + > +#define RESETTABLE_MAX_COUNT 50 > + > +#define RESETTABLE_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE) Since this is an interface and not a complete class, we should name it TYPE_RESETTABLE_INTERFACE. We're rather inconsistent about naming of interfaces (in the codebase you can find "_INTERFACE suffix", "_IF suffix" and "no suffix"). I think _INTERFACE suffix is best of these. (There's some discussion of this in this mailing list thread: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg03840.html -- the idea is that it avoids confusion between whether an implementation of the type needs to be a subclass of it or if it has to be added as an interface to some other type.) > + > +static void resettable_init_reset(Object *obj, bool cold); > + > +static bool resettable_class_check(ResettableClass *rc) > +{ > + if (!rc->set_cold) { > + return false; > + } > + if (!rc->set_hold_needed) { > + return false; > + } > + if (!rc->increment_count) { > + return false; > + } > + if (!rc->decrement_count) { > + return false; > + } > + if (!rc->get_count) { > + return false; > + } > + return true; > +} I don't think we really need to do this -- this is only used as an assert(), and the code is in any case going to exercise all the methods. Plus we only implement these methods in a couple of classes and we don't expect a lot of new implementations of them. > + > +static void resettable_foreach_child(ResettableClass *rc, > + Object *obj, > + void (*func)(Object *)) > +{ > + if (rc->foreach_child) { > + rc->foreach_child(obj, func); > + } > +} > + > +static void resettable_init_cold_reset(Object *obj) > +{ > + resettable_init_reset(obj, true); > +} > + > +static void resettable_init_warm_reset(Object *obj) > +{ > + resettable_init_reset(obj, false); > +} > + > +static void resettable_init_reset(Object *obj, bool cold) > +{ > + void (*func)(Object *); > + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > + uint32_t count; > + bool action_needed = false; > + bool prev_cold; > + > + assert(resettable_class_check(rc)); > + > + count = rc->increment_count(obj); > + /* this assert is here to catch an eventual reset loop */ > + assert(count <= RESETTABLE_MAX_COUNT); I think I understand what this assert is for, but a slightly expanded comment would help. > + > + /* > + * only take action if: > + * + we really enter reset for the 1st time > + * + or we are in warm reset and start a cold one > + */ > + prev_cold = rc->set_cold(obj, cold); > + if (count == 1) { > + action_needed = true; > + } else if (cold && !prev_cold) { > + action_needed = true; > + } > + trace_resettable_phase_init(obj, object_get_typename(obj), cold, count, > + action_needed); > + > + /* exec init phase */ > + if (action_needed) { > + rc->set_hold_needed(obj, true); > + if (rc->phases.init) { > + rc->phases.init(obj); > + } > + } > + > + /* > + * handle the children even if action_needed is at false so that > + * children counts are incremented too > + */ > + func = cold ? resettable_init_cold_reset : resettable_init_warm_reset; > + resettable_foreach_child(rc, obj, func); > + trace_resettable_phase_init_end(obj); > +} > + > +static void resettable_hold_reset(Object *obj) > +{ > + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > + bool hold_needed; > + > + assert(resettable_class_check(rc)); > + trace_resettable_phase_hold(obj, object_get_typename(obj)); > + > + /* handle children first */ > + resettable_foreach_child(rc, obj, resettable_hold_reset); > + > + /* exec hold phase */ > + hold_needed = rc->set_hold_needed(obj, false); > + if (hold_needed) { > + if (rc->phases.hold) { > + rc->phases.hold(obj); > + } > + } > + trace_resettable_phase_hold_end(obj, hold_needed); > +} > + > +static void resettable_exit_reset(Object *obj) > +{ > + uint32_t count; > + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); > + > + assert(resettable_class_check(rc)); > + trace_resettable_phase_exit(obj, object_get_typename(obj)); > + > + resettable_foreach_child(rc, obj, resettable_deassert_reset); > + > + count = rc->get_count(obj); > + /* > + * we could assert that count > 0 but there are some corner cases > + * where we prefer to let it go as it is probably harmless. > + * For example: if there is reset support addition between > + * hosts when doing a migration. We may do such things as > + * deassert a non-existing reset. > + */ > + if (count > 0) { > + count = rc->decrement_count(obj); > + } else { > + trace_resettable_count_underflow(obj); > + } > + if (count == 0) { > + if (rc->phases.exit) { > + rc->phases.exit(obj); > + } > + } > + trace_resettable_phase_exit_end(obj, count); > +} > + > +void resettable_assert_reset(Object *obj, bool cold) > +{ > + trace_resettable_reset_assert(obj, object_get_typename(obj), cold); > + resettable_init_reset(obj, cold); > + resettable_hold_reset(obj); > +} > + > +void resettable_deassert_reset(Object *obj) > +{ > + trace_resettable_reset_deassert(obj, object_get_typename(obj)); > + resettable_exit_reset(obj); > +} > + > +void resettable_reset(Object *obj, bool cold) > +{ > + trace_resettable_reset(obj, object_get_typename(obj), cold); > + resettable_assert_reset(obj, cold); > + resettable_deassert_reset(obj); > +} > + > +void resettable_reset_warm_fn(void *opaque) > +{ > + resettable_reset((Object *) opaque, false); > +} > + > +void resettable_reset_cold_fn(void *opaque) > +{ > + resettable_reset((Object *) opaque, true); > +} > + > +void resettable_class_set_parent_reset_phases(ResettableClass *rc, > + ResettableInitPhase init, > + ResettableHoldPhase hold, > + ResettableExitPhase exit, > + ResettablePhases *parent_phases) > +{ > + *parent_phases = rc->phases; > + if (init) { > + rc->phases.init = init; > + } > + if (hold) { > + rc->phases.hold = hold; > + } > + if (exit) { > + rc->phases.exit = exit; > + } > +} > + > +static const TypeInfo resettable_interface_info = { > + .name = TYPE_RESETTABLE, > + .parent = TYPE_INTERFACE, > + .class_size = sizeof(ResettableClass), > +}; > + > +static void reset_register_types(void) > +{ > + type_register_static(&resettable_interface_info); > +} > + > +type_init(reset_register_types) > diff --git a/hw/core/trace-events b/hw/core/trace-events > new file mode 100644 > index 0000000000..489d96d445 > --- /dev/null > +++ b/hw/core/trace-events > @@ -0,0 +1,39 @@ > +# See docs/devel/tracing.txt for syntax documentation. > +# > +# This file is processed by the tracetool script during the build. > +# > +# To add a new trace event: > +# > +# 1. Choose a name for the trace event. Declare its arguments and format > +# string. > +# > +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() -> > +# trace_multiwrite_cb(). The source file must #include "trace.h". > +# > +# Format of a trace event: > +# > +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>" > +# > +# Example: g_malloc(size_t size) "size %zu" > +# > +# The "disable" keyword will build without the trace event. > +# > +# The <name> must be a valid as a C function name. > +# > +# Types should be standard C types. Use void * for pointers because the trace > +# system may not have the necessary headers included. > +# > +# The <format-string> should be a sprintf()-compatible format string. > + > +# resettable.c > +resettable_reset(void *obj, const char *type, int cold) "obj=%p(%s) cold=%d" > +resettable_reset_assert(void *obj, const char *type, int cold) "obj=%p(%s) cold=%d" > +resettable_reset_deassert(void *obj, const char *type) "obj=%p(%s)" > +resettable_reset_deassert_end(void *obj) "obj=%p" > +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d" > +resettable_phase_init_end(void *obj) "obj=%p" > +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)" > +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d" > +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)" > +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32 > +resettable_count_underflow(void *obj) "obj=%p" > diff --git a/include/hw/resettable.h b/include/hw/resettable.h > new file mode 100644 > index 0000000000..e617a8e875 > --- /dev/null > +++ b/include/hw/resettable.h > @@ -0,0 +1,126 @@ > +#ifndef HW_RESETTABLE_H > +#define HW_RESETTABLE_H > + > +#include "qom/object.h" > + > +#define TYPE_RESETTABLE "resettable" > + > +#define RESETTABLE_CLASS(class) \ > + OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE) > + > +/* > + * ResettableClass: > + * Interface for resettable objects. > + * > + * The reset operation is divided in several phases each represented by a > + * method. > + * > + * Each Ressetable must maintain a reset counter in its state, 3 methods allows "resettable" > + * to interact with it. I think we could improve this comment to be a bit clearer about who we expect to implement which methods. Something like: /* * ResettableClass: * Interface for resettable objects. * * See docs/devel/reset.rst for more detailed information about * how QEMU models reset. * * 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: * - a reset count, which is incremented when the reset operation * starts and decremented when it finishes * - a 'cold' flag, which tracks whether the in-progress reset is * a warm reset or a cold reset * - a 'hold_needed' flag, which indicates that we need to * invoke the 'hold' phase handler for this object * The base class implementation of the interface provides this * state and implements the associated methods: set_cold, * set_hold_needed, get_count, increment_count and decrement_count. * * Concrete object implementations (typically specific devices * such as a UART model) should provide the functions * for the phases.init, phases.hold and phases.exit methods, which * they can set in their class init function, either directly or * by calling resettable_class_set_parent_reset_phases(). * The phase methods are guaranteed to only only ever be called once * for any reset event, in the order 'init', 'hold', 'exit'. * An object will always move quickly from 'init' 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 * 'init' method has been called. * * Users of a resettable object should not call these methods * directly, but instead use the functions resettable_assert_reset(), * resettable_deassert_reset() or resettable_reset(). > + * > + * @phases.init: should reset local state only. Takes a bool @cold argument > + * specifying whether the reset is cold or warm. It must not do side-effect > + * on others objects. This says that phases.init takes a 'cold' argument, but the prototype doesn't have one. "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." > + * > + * @phases.hold: side-effects action on others objects due to staying in a > + * resetting state. "This phase is called for entry into reset, once every object in the system which is being reset has had its @phases.init method called. At this point devices can do actions that affect other objects." > + * > + * @phases.exit: leave the reset state, may do side-effects action on others > + * objects. "This phase is called when the object leaves the reset state. Actions affecting other objects are permitted." > + * > + * @set_cold: Set whether the current reset is cold or warm. Return the > + * previous flag value. Return value has no meaning if @get_count returns > + * a zero value. > + * > + * @set_hold_needed: Set hold_needed flag. Return the previous flag value. > + * > + * @get_count: Get the current reset count > + * @increment_count: Increment the reset count, returns the new count > + * @decrement_count: decrement the reset count, returns the new count > + * > + * @foreach_child: Executes a given function on every Resettable child. > + * A child is not a QOM child, but a child a reset meaning. "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." > + */ > +typedef void (*ResettableInitPhase)(Object *obj); > +typedef void (*ResettableHoldPhase)(Object *obj); > +typedef void (*ResettableExitPhase)(Object *obj); > +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; > + > + struct ResettablePhases { > + ResettableInitPhase init; > + ResettableHoldPhase hold; > + ResettableExitPhase exit; > + } phases; > + > + ResettableSetCold set_cold; > + ResettableSetHoldNeeded set_hold_needed; > + ResettableGetCount get_count; > + ResettableIncrementCount increment_count; > + ResettableDecrementCount decrement_count; > + ResettableForeachChild foreach_child; > +} ResettableClass; > +typedef struct ResettablePhases ResettablePhases; > + > +/** > + * resettable_assert_reset: "Put the object into reset, and hold it there until the caller later calls resettable_deassert_reset()." > + * Increments the reset count and executes the init and hold phases. > + * Each time resettable_assert_reset is called, resettable_deassert_reset > + * must eventually be called once. > + * It will also impact reset children. "This will reset the specified object and all of its reset children." > + * > + * @obj object to reset, must implement Resettable interface. > + * @cold boolean indicating the type of reset (cold or warm) > + */ > +void resettable_assert_reset(Object *obj, bool cold); > + > +/** > + * resettable_deassert_reset: "Take the object out of reset." > + * Decrements the reset count by one and executes the exit phase if it hits > + * zero. > + * It will also impact reset children. > + * > + * @obj object to reset, must implement Resettable interface. > + */ > +void resettable_deassert_reset(Object *obj); > + > +/** > + * resettable_reset: > + * Calling this function is equivalent to call @assert_reset then "to calling" > + * @deassert_reset. > + */ > +void resettable_reset(Object *obj, bool cold); > + > +/** > + * resettable_reset_warm_fn: > + * Helper to call resettable_reset(opaque, false) > + */ > +void resettable_reset_warm_fn(void *opaque); > + > +/** > + * resettable_reset_cold_fn: > + * Helper to call resettable_reset(opaque, true) > + */ > +void resettable_reset_cold_fn(void *opaque); > + > +/** > + * resettable_class_set_parent_reset_phases: > + * > + * Save @rc current reset phases into @parent_phases and override @rc phases > + * by the given new methods (@init, @hold and @exit). > + * Each phase is overriden only if the new one is not NULL allowing to "overridden" > + * override a subset of phases. > + */ > +void resettable_class_set_parent_reset_phases(ResettableClass *rc, > + ResettableInitPhase init, > + ResettableHoldPhase hold, > + ResettableExitPhase exit, > + ResettablePhases *parent_phases); > + > +#endif This function name is quite long -- I think we could reasonably call it resettable_class_set_parent_phases(). thanks -- PMM
On 8/7/19 4:20 PM, Peter Maydell wrote: > On Mon, 29 Jul 2019 at 15:58, Damien Hedde <damien.hedde@greensocs.com> wrote: >> >> This commit defines an interface allowing multi-phase reset. >> The phases are INIT, HOLD and EXIT. Each phase has an associated method >> in the class. >> >> The reset of a Resettable is controlled with 2 functions: >> - resettable_assert_reset which starts the reset operation. >> - resettable_deassert_reset which ends the reset operation. >> >> There is also a `resettable_reset` helper function which does assert then >> deassert allowing to do a proper reset in one call. >> >> In addition, two functions, *resettable_reset_cold_fn* and >> *resettable_reset_warm_fn*, are defined. They take only an opaque argument >> (for the Resettable object) and can be used as callbacks. >> >> The Resettable interface is "reentrant", _assert_ can be called several >> times and _deasert_ must be called the same number of times so that the > > deassert > >> Resettable leave reset state. It is supported by keeping a counter of >> the current number of _assert_ calls. The counter maintainance is done >> though 3 methods get/increment/decrement_count. A method set_cold is used >> to set the cold flag of the reset. Another method set_host_needed is used >> to ensure hold phase is executed only if required. >> >> Reset hierarchy is also supported. Each Resettable may have >> sub-Resettable objects. When resetting a Resettable, it is propagated to >> its children using the foreach_child method. >> >> When entering reset, init phases are executed parent-to-child order then >> hold phases are executed child-parent order. > > Why do we execute the hold phase in child-to-parent order? I would > have expected this to follow the same order as the init phase. No particular reason, I just thought it was better that way. But I don't have a use case where it matters. If we do only an assert_reset, then top-level phases are called first and last: parent's init child A's init child_B's init child_A's hold child_B's hold parent's hold I can switch it. > >> When leaving reset, exit phases are executed in child-to-parent order. >> This will allow to replace current qdev_reset mechanism by this interface >> without side-effects on reset order. > > It makes sense that the exit phase is child-to-parent. > >> Note: I used an uint32 for the count. This match the type already used >> in the existing resetting counter in hw/scsi/vmw_pvscsi.c for the >> PVSCSIState. >> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >> --- >> Makefile.objs | 1 + >> hw/core/Makefile.objs | 1 + >> hw/core/resettable.c | 220 ++++++++++++++++++++++++++++++++++++++++ >> hw/core/trace-events | 39 +++++++ >> include/hw/resettable.h | 126 +++++++++++++++++++++++ >> 5 files changed, 387 insertions(+) >> create mode 100644 hw/core/resettable.c >> create mode 100644 hw/core/trace-events >> create mode 100644 include/hw/resettable.h >> >> diff --git a/Makefile.objs b/Makefile.objs >> index 6a143dcd57..a723a47e14 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/hw/core/Makefile.objs b/hw/core/Makefile.objs >> index f8481d959f..d9234aa98a 100644 >> --- a/hw/core/Makefile.objs >> +++ b/hw/core/Makefile.objs >> @@ -1,6 +1,7 @@ >> # core qdev-related obj files, also used by *-user: >> common-obj-y += qdev.o qdev-properties.o >> common-obj-y += bus.o reset.o >> +common-obj-y += resettable.o >> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o >> common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o >> # irq.o needed for qdev GPIO handling: >> diff --git a/hw/core/resettable.c b/hw/core/resettable.c >> new file mode 100644 >> index 0000000000..d7d631ce8b >> --- /dev/null >> +++ b/hw/core/resettable.c >> @@ -0,0 +1,220 @@ >> +/* >> + * Resettable interface. >> + * >> + * 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. >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "qemu/module.h" >> +#include "hw/resettable.h" >> +#include "trace.h" >> + >> +#define RESETTABLE_MAX_COUNT 50 >> + >> +#define RESETTABLE_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE) > > Since this is an interface and not a complete class, > we should name it TYPE_RESETTABLE_INTERFACE. We're rather > inconsistent about naming of interfaces (in the codebase you > can find "_INTERFACE suffix", "_IF suffix" and "no suffix"). > I think _INTERFACE suffix is best of these. > (There's some discussion of this in this mailing list thread: > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg03840.html > -- the idea is that it avoids confusion between whether an > implementation of the type needs to be a subclass of it or > if it has to be added as an interface to some other type.) > >> + >> +static void resettable_init_reset(Object *obj, bool cold); >> + >> +static bool resettable_class_check(ResettableClass *rc) >> +{ >> + if (!rc->set_cold) { >> + return false; >> + } >> + if (!rc->set_hold_needed) { >> + return false; >> + } >> + if (!rc->increment_count) { >> + return false; >> + } >> + if (!rc->decrement_count) { >> + return false; >> + } >> + if (!rc->get_count) { >> + return false; >> + } >> + return true; >> +} > > I don't think we really need to do this -- this is only used > as an assert(), and the code is in any case going to exercise > all the methods. Plus we only implement these methods in a couple > of classes and we don't expect a lot of new implementations of them. > >> + >> +static void resettable_foreach_child(ResettableClass *rc, >> + Object *obj, >> + void (*func)(Object *)) >> +{ >> + if (rc->foreach_child) { >> + rc->foreach_child(obj, func); >> + } >> +} >> + >> +static void resettable_init_cold_reset(Object *obj) >> +{ >> + resettable_init_reset(obj, true); >> +} >> + >> +static void resettable_init_warm_reset(Object *obj) >> +{ >> + resettable_init_reset(obj, false); >> +} >> + >> +static void resettable_init_reset(Object *obj, bool cold) >> +{ >> + void (*func)(Object *); >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); >> + uint32_t count; >> + bool action_needed = false; >> + bool prev_cold; >> + >> + assert(resettable_class_check(rc)); >> + >> + count = rc->increment_count(obj); >> + /* this assert is here to catch an eventual reset loop */ >> + assert(count <= RESETTABLE_MAX_COUNT); > > I think I understand what this assert is for, but a slightly > expanded comment would help. is this better ? /* * We limit the count to an arbitrary "big" value. The value is big * enough not to be triggered nominally. * The assert will catch a cycle in the reset tree which triggers * an infinite loop. */ Maybe I should drop the macro here, it will be more explicit. > >> + >> + /* >> + * only take action if: >> + * + we really enter reset for the 1st time >> + * + or we are in warm reset and start a cold one >> + */ >> + prev_cold = rc->set_cold(obj, cold); >> + if (count == 1) { >> + action_needed = true; >> + } else if (cold && !prev_cold) { >> + action_needed = true; >> + } >> + trace_resettable_phase_init(obj, object_get_typename(obj), cold, count, >> + action_needed); >> + >> + /* exec init phase */ >> + if (action_needed) { >> + rc->set_hold_needed(obj, true); >> + if (rc->phases.init) { >> + rc->phases.init(obj); >> + } >> + } >> + >> + /* >> + * handle the children even if action_needed is at false so that >> + * children counts are incremented too >> + */ >> + func = cold ? resettable_init_cold_reset : resettable_init_warm_reset; >> + resettable_foreach_child(rc, obj, func); >> + trace_resettable_phase_init_end(obj); >> +} >> + >> +static void resettable_hold_reset(Object *obj) >> +{ >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); >> + bool hold_needed; >> + >> + assert(resettable_class_check(rc)); >> + trace_resettable_phase_hold(obj, object_get_typename(obj)); >> + >> + /* handle children first */ >> + resettable_foreach_child(rc, obj, resettable_hold_reset); >> + >> + /* exec hold phase */ >> + hold_needed = rc->set_hold_needed(obj, false); >> + if (hold_needed) { >> + if (rc->phases.hold) { >> + rc->phases.hold(obj); >> + } >> + } >> + trace_resettable_phase_hold_end(obj, hold_needed); >> +} >> + >> +static void resettable_exit_reset(Object *obj) >> +{ >> + uint32_t count; >> + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); >> + >> + assert(resettable_class_check(rc)); >> + trace_resettable_phase_exit(obj, object_get_typename(obj)); >> + >> + resettable_foreach_child(rc, obj, resettable_deassert_reset); >> + >> + count = rc->get_count(obj); >> + /* >> + * we could assert that count > 0 but there are some corner cases >> + * where we prefer to let it go as it is probably harmless. >> + * For example: if there is reset support addition between >> + * hosts when doing a migration. We may do such things as >> + * deassert a non-existing reset. >> + */ >> + if (count > 0) { >> + count = rc->decrement_count(obj); >> + } else { >> + trace_resettable_count_underflow(obj); >> + } >> + if (count == 0) { >> + if (rc->phases.exit) { >> + rc->phases.exit(obj); >> + } >> + } >> + trace_resettable_phase_exit_end(obj, count); >> +} >> + >> +void resettable_assert_reset(Object *obj, bool cold) >> +{ >> + trace_resettable_reset_assert(obj, object_get_typename(obj), cold); >> + resettable_init_reset(obj, cold); >> + resettable_hold_reset(obj); >> +} >> + >> +void resettable_deassert_reset(Object *obj) >> +{ >> + trace_resettable_reset_deassert(obj, object_get_typename(obj)); >> + resettable_exit_reset(obj); >> +} >> + >> +void resettable_reset(Object *obj, bool cold) >> +{ >> + trace_resettable_reset(obj, object_get_typename(obj), cold); >> + resettable_assert_reset(obj, cold); >> + resettable_deassert_reset(obj); >> +} >> + >> +void resettable_reset_warm_fn(void *opaque) >> +{ >> + resettable_reset((Object *) opaque, false); >> +} >> + >> +void resettable_reset_cold_fn(void *opaque) >> +{ >> + resettable_reset((Object *) opaque, true); >> +} >> + >> +void resettable_class_set_parent_reset_phases(ResettableClass *rc, >> + ResettableInitPhase init, >> + ResettableHoldPhase hold, >> + ResettableExitPhase exit, >> + ResettablePhases *parent_phases) >> +{ >> + *parent_phases = rc->phases; >> + if (init) { >> + rc->phases.init = init; >> + } >> + if (hold) { >> + rc->phases.hold = hold; >> + } >> + if (exit) { >> + rc->phases.exit = exit; >> + } >> +} >> + >> +static const TypeInfo resettable_interface_info = { >> + .name = TYPE_RESETTABLE, >> + .parent = TYPE_INTERFACE, >> + .class_size = sizeof(ResettableClass), >> +}; >> + >> +static void reset_register_types(void) >> +{ >> + type_register_static(&resettable_interface_info); >> +} >> + >> +type_init(reset_register_types) >> diff --git a/hw/core/trace-events b/hw/core/trace-events >> new file mode 100644 >> index 0000000000..489d96d445 >> --- /dev/null >> +++ b/hw/core/trace-events >> @@ -0,0 +1,39 @@ >> +# See docs/devel/tracing.txt for syntax documentation. >> +# >> +# This file is processed by the tracetool script during the build. >> +# >> +# To add a new trace event: >> +# >> +# 1. Choose a name for the trace event. Declare its arguments and format >> +# string. >> +# >> +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() -> >> +# trace_multiwrite_cb(). The source file must #include "trace.h". >> +# >> +# Format of a trace event: >> +# >> +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>" >> +# >> +# Example: g_malloc(size_t size) "size %zu" >> +# >> +# The "disable" keyword will build without the trace event. >> +# >> +# The <name> must be a valid as a C function name. >> +# >> +# Types should be standard C types. Use void * for pointers because the trace >> +# system may not have the necessary headers included. >> +# >> +# The <format-string> should be a sprintf()-compatible format string. >> + >> +# resettable.c >> +resettable_reset(void *obj, const char *type, int cold) "obj=%p(%s) cold=%d" >> +resettable_reset_assert(void *obj, const char *type, int cold) "obj=%p(%s) cold=%d" >> +resettable_reset_deassert(void *obj, const char *type) "obj=%p(%s)" >> +resettable_reset_deassert_end(void *obj) "obj=%p" >> +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d" >> +resettable_phase_init_end(void *obj) "obj=%p" >> +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)" >> +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d" >> +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)" >> +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32 >> +resettable_count_underflow(void *obj) "obj=%p" >> diff --git a/include/hw/resettable.h b/include/hw/resettable.h >> new file mode 100644 >> index 0000000000..e617a8e875 >> --- /dev/null >> +++ b/include/hw/resettable.h >> @@ -0,0 +1,126 @@ >> +#ifndef HW_RESETTABLE_H >> +#define HW_RESETTABLE_H >> + >> +#include "qom/object.h" >> + >> +#define TYPE_RESETTABLE "resettable" >> + >> +#define RESETTABLE_CLASS(class) \ >> + OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE) >> + >> +/* >> + * ResettableClass: >> + * Interface for resettable objects. >> + * >> + * The reset operation is divided in several phases each represented by a >> + * method. >> + * >> + * Each Ressetable must maintain a reset counter in its state, 3 methods allows > > "resettable" > >> + * to interact with it. > > I think we could improve this comment to be a bit clearer about > who we expect to implement which methods. Something like: > > /* > * ResettableClass: > * Interface for resettable objects. > * > * See docs/devel/reset.rst for more detailed information about > * how QEMU models reset. > * > * 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: > * - a reset count, which is incremented when the reset operation > * starts and decremented when it finishes > * - a 'cold' flag, which tracks whether the in-progress reset is > * a warm reset or a cold reset > * - a 'hold_needed' flag, which indicates that we need to > * invoke the 'hold' phase handler for this object > * The base class implementation of the interface provides this > * state and implements the associated methods: set_cold, > * set_hold_needed, get_count, increment_count and decrement_count. > * > * Concrete object implementations (typically specific devices > * such as a UART model) should provide the functions > * for the phases.init, phases.hold and phases.exit methods, which > * they can set in their class init function, either directly or > * by calling resettable_class_set_parent_reset_phases(). > * The phase methods are guaranteed to only only ever be called once > * for any reset event, in the order 'init', 'hold', 'exit'. > * An object will always move quickly from 'init' 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 > * 'init' method has been called. > * > * Users of a resettable object should not call these methods > * directly, but instead use the functions resettable_assert_reset(), > * resettable_deassert_reset() or resettable_reset(). > > >> + * >> + * @phases.init: should reset local state only. Takes a bool @cold argument >> + * specifying whether the reset is cold or warm. It must not do side-effect >> + * on others objects. > > This says that phases.init takes a 'cold' argument, but the prototype > doesn't have one. I finally removed the argument and forgot to change this bit. > > "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." > >> + * >> + * @phases.hold: side-effects action on others objects due to staying in a >> + * resetting state. > > "This phase is called for entry into reset, once every object in the > system which is being reset has had its @phases.init method called. > At this point devices can do actions that affect other objects." > > >> + * >> + * @phases.exit: leave the reset state, may do side-effects action on others >> + * objects. > > "This phase is called when the object leaves the reset state. Actions > affecting other objects are permitted." > >> + * >> + * @set_cold: Set whether the current reset is cold or warm. Return the >> + * previous flag value. Return value has no meaning if @get_count returns >> + * a zero value. >> + * >> + * @set_hold_needed: Set hold_needed flag. Return the previous flag value. >> + * >> + * @get_count: Get the current reset count >> + * @increment_count: Increment the reset count, returns the new count >> + * @decrement_count: decrement the reset count, returns the new count >> + * >> + * @foreach_child: Executes a given function on every Resettable child. >> + * A child is not a QOM child, but a child a reset meaning. > > "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." > >> + */ >> +typedef void (*ResettableInitPhase)(Object *obj); >> +typedef void (*ResettableHoldPhase)(Object *obj); >> +typedef void (*ResettableExitPhase)(Object *obj); >> +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; >> + >> + struct ResettablePhases { >> + ResettableInitPhase init; >> + ResettableHoldPhase hold; >> + ResettableExitPhase exit; >> + } phases; >> + >> + ResettableSetCold set_cold; >> + ResettableSetHoldNeeded set_hold_needed; >> + ResettableGetCount get_count; >> + ResettableIncrementCount increment_count; >> + ResettableDecrementCount decrement_count; >> + ResettableForeachChild foreach_child; >> +} ResettableClass; >> +typedef struct ResettablePhases ResettablePhases; >> + >> +/** >> + * resettable_assert_reset: > > "Put the object into reset, and hold it there until > the caller later calls resettable_deassert_reset()." > >> + * Increments the reset count and executes the init and hold phases. >> + * Each time resettable_assert_reset is called, resettable_deassert_reset >> + * must eventually be called once. >> + * It will also impact reset children. > > "This will reset the specified object and all of its reset children." > >> + * >> + * @obj object to reset, must implement Resettable interface. >> + * @cold boolean indicating the type of reset (cold or warm) >> + */ >> +void resettable_assert_reset(Object *obj, bool cold); >> + >> +/** >> + * resettable_deassert_reset: > > "Take the object out of reset." > >> + * Decrements the reset count by one and executes the exit phase if it hits >> + * zero. >> + * It will also impact reset children. >> + * >> + * @obj object to reset, must implement Resettable interface. >> + */ >> +void resettable_deassert_reset(Object *obj); >> + >> +/** >> + * resettable_reset: >> + * Calling this function is equivalent to call @assert_reset then > > "to calling" > >> + * @deassert_reset. >> + */ >> +void resettable_reset(Object *obj, bool cold); >> + >> +/** >> + * resettable_reset_warm_fn: >> + * Helper to call resettable_reset(opaque, false) >> + */ >> +void resettable_reset_warm_fn(void *opaque); >> + >> +/** >> + * resettable_reset_cold_fn: >> + * Helper to call resettable_reset(opaque, true) >> + */ >> +void resettable_reset_cold_fn(void *opaque); >> + >> +/** >> + * resettable_class_set_parent_reset_phases: >> + * >> + * Save @rc current reset phases into @parent_phases and override @rc phases >> + * by the given new methods (@init, @hold and @exit). >> + * Each phase is overriden only if the new one is not NULL allowing to > > "overridden" > >> + * override a subset of phases. >> + */ >> +void resettable_class_set_parent_reset_phases(ResettableClass *rc, >> + ResettableInitPhase init, >> + ResettableHoldPhase hold, >> + ResettableExitPhase exit, >> + ResettablePhases *parent_phases); >> + >> +#endif > > This function name is quite long -- I think we could reasonably > call it resettable_class_set_parent_phases(). > > thanks > -- PMM >
On Thu, Aug 01, 2019 at 11:35:20AM +0200, Damien Hedde wrote: > > > On 7/31/19 7:46 AM, David Gibson wrote: > > On Tue, Jul 30, 2019 at 04:08:59PM +0200, Damien Hedde wrote: > >> > >> On 7/30/19 3:59 PM, Peter Maydell wrote: > >>> On Tue, 30 Jul 2019 at 14:56, Cornelia Huck <cohuck@redhat.com> wrote: > >>>> > >>>> On Tue, 30 Jul 2019 14:44:21 +0100 > >>>> Peter Maydell <peter.maydell@linaro.org> wrote: > >>>> > >>>>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck <cohuck@redhat.com> wrote: > >>>>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is > >>>>>> supposed to be... can you add a definition/guideline somewhere? > >>>>> > >>>>> Generally "cold" reset is "power on" and "warm" is "we were already > >>>>> powered-on, but somebody flipped a reset line somewhere". > >>>> > >>>> Ok, that makes sense... my main concern is to distinguish that in a > >>>> generic way, as it is a generic interface. What about adding something > >>>> like: > >>>> > >>>> "A 'cold' reset means that the object to be reset is initially reset; a 'warm' > >>>> reset means that the object to be reset has already been initialized." > >>>> > >>>> Or is that again too generic? > >>> > >>> I think it doesn't quite capture the idea -- an object can have already > >>> been reset and then get a 'cold' reset: this is like having a powered-on > >>> machine and then power-cycling it. > >>> > >>> The 'warm' reset is the vaguer one, because the specific behaviour > >>> is somewhat device-dependent (many devices might not have any > >>> difference from 'cold' reset, for those that do the exact detail > >>> of what doesn't get reset on warm-reset will vary). But every > >>> device should have some kind of "as if you power-cycled it" (or > >>> for QEMU, "go back to the same state as if you just started QEMU on the > >>> command line"). Our current "reset" method is really cold-reset. > >>> > >> > >> Exactly. In the following patches, I've tried to replace existing reset > >> calls by cold or warm reset depending on whether: > >> + it is called through the main system reset -> cold > >> + it is called during normal life-time -> warm > >> > >> But I definitely can add some docs/comments to better explain that. > > > > Hrm, that helps, but it still seems pretty vague to me. > > > > It's not really my call, but building the concept of warm versus cold > > resets into such a generic interface seems pretty dubios to me. While > > it's moderately common for things to have a notion of warm versus cold > > reset it's certainly not universal. There are many devices where > > there's no meaningful difference between the two. There are some > > devices where there are > 2 different types of reset suitable for > > various different situations. > > > > Is there any way this could work with it usually just presenting the > > cold reset (which is the closest to a universal concept), and any warm > > or additional resets are handled buy a different instance of the > > Resettable interface? > > > > In my current implementation, cold/warm is only a question of setting a > flag before calling the reset methods. I rely and the reset methods to > check that flag if necessary. The feature can be added/removed without > pain (if we stick with device_reset to do a cold one). So if it's > better, I can put it aside for now. > > IMO handling warm reset with another interface is probably not a good > idea because it will need another load of methods. I was thinking of a different instance of the same interface, not a new interface. But on consideration that probably means dummy objects so that's also ugly. Here's another way to look at things though: I can see why a common interface for cold resets is useful; we use it during system resets and bus resets and so forth. But AIUI, in order to do a warm reset whatever is initiating it is going to need to know what a warm reset means for this device - in which case it doesn't really need a uniform interface for it. So, would we be better off with helper functions to make implementing a multi-phase reset easy for devices that do have one or more notions of a warm reset.
diff --git a/Makefile.objs b/Makefile.objs index 6a143dcd57..a723a47e14 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/hw/core/Makefile.objs b/hw/core/Makefile.objs index f8481d959f..d9234aa98a 100644 --- a/hw/core/Makefile.objs +++ b/hw/core/Makefile.objs @@ -1,6 +1,7 @@ # core qdev-related obj files, also used by *-user: common-obj-y += qdev.o qdev-properties.o common-obj-y += bus.o reset.o +common-obj-y += resettable.o common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o # irq.o needed for qdev GPIO handling: diff --git a/hw/core/resettable.c b/hw/core/resettable.c new file mode 100644 index 0000000000..d7d631ce8b --- /dev/null +++ b/hw/core/resettable.c @@ -0,0 +1,220 @@ +/* + * Resettable interface. + * + * 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. + */ + +#include "qemu/osdep.h" +#include "qemu/module.h" +#include "hw/resettable.h" +#include "trace.h" + +#define RESETTABLE_MAX_COUNT 50 + +#define RESETTABLE_GET_CLASS(obj) \ + OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE) + +static void resettable_init_reset(Object *obj, bool cold); + +static bool resettable_class_check(ResettableClass *rc) +{ + if (!rc->set_cold) { + return false; + } + if (!rc->set_hold_needed) { + return false; + } + if (!rc->increment_count) { + return false; + } + if (!rc->decrement_count) { + return false; + } + if (!rc->get_count) { + return false; + } + return true; +} + +static void resettable_foreach_child(ResettableClass *rc, + Object *obj, + void (*func)(Object *)) +{ + if (rc->foreach_child) { + rc->foreach_child(obj, func); + } +} + +static void resettable_init_cold_reset(Object *obj) +{ + resettable_init_reset(obj, true); +} + +static void resettable_init_warm_reset(Object *obj) +{ + resettable_init_reset(obj, false); +} + +static void resettable_init_reset(Object *obj, bool cold) +{ + void (*func)(Object *); + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); + uint32_t count; + bool action_needed = false; + bool prev_cold; + + assert(resettable_class_check(rc)); + + count = rc->increment_count(obj); + /* this assert is here to catch an eventual reset loop */ + assert(count <= RESETTABLE_MAX_COUNT); + + /* + * only take action if: + * + we really enter reset for the 1st time + * + or we are in warm reset and start a cold one + */ + prev_cold = rc->set_cold(obj, cold); + if (count == 1) { + action_needed = true; + } else if (cold && !prev_cold) { + action_needed = true; + } + trace_resettable_phase_init(obj, object_get_typename(obj), cold, count, + action_needed); + + /* exec init phase */ + if (action_needed) { + rc->set_hold_needed(obj, true); + if (rc->phases.init) { + rc->phases.init(obj); + } + } + + /* + * handle the children even if action_needed is at false so that + * children counts are incremented too + */ + func = cold ? resettable_init_cold_reset : resettable_init_warm_reset; + resettable_foreach_child(rc, obj, func); + trace_resettable_phase_init_end(obj); +} + +static void resettable_hold_reset(Object *obj) +{ + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); + bool hold_needed; + + assert(resettable_class_check(rc)); + trace_resettable_phase_hold(obj, object_get_typename(obj)); + + /* handle children first */ + resettable_foreach_child(rc, obj, resettable_hold_reset); + + /* exec hold phase */ + hold_needed = rc->set_hold_needed(obj, false); + if (hold_needed) { + if (rc->phases.hold) { + rc->phases.hold(obj); + } + } + trace_resettable_phase_hold_end(obj, hold_needed); +} + +static void resettable_exit_reset(Object *obj) +{ + uint32_t count; + ResettableClass *rc = RESETTABLE_GET_CLASS(obj); + + assert(resettable_class_check(rc)); + trace_resettable_phase_exit(obj, object_get_typename(obj)); + + resettable_foreach_child(rc, obj, resettable_deassert_reset); + + count = rc->get_count(obj); + /* + * we could assert that count > 0 but there are some corner cases + * where we prefer to let it go as it is probably harmless. + * For example: if there is reset support addition between + * hosts when doing a migration. We may do such things as + * deassert a non-existing reset. + */ + if (count > 0) { + count = rc->decrement_count(obj); + } else { + trace_resettable_count_underflow(obj); + } + if (count == 0) { + if (rc->phases.exit) { + rc->phases.exit(obj); + } + } + trace_resettable_phase_exit_end(obj, count); +} + +void resettable_assert_reset(Object *obj, bool cold) +{ + trace_resettable_reset_assert(obj, object_get_typename(obj), cold); + resettable_init_reset(obj, cold); + resettable_hold_reset(obj); +} + +void resettable_deassert_reset(Object *obj) +{ + trace_resettable_reset_deassert(obj, object_get_typename(obj)); + resettable_exit_reset(obj); +} + +void resettable_reset(Object *obj, bool cold) +{ + trace_resettable_reset(obj, object_get_typename(obj), cold); + resettable_assert_reset(obj, cold); + resettable_deassert_reset(obj); +} + +void resettable_reset_warm_fn(void *opaque) +{ + resettable_reset((Object *) opaque, false); +} + +void resettable_reset_cold_fn(void *opaque) +{ + resettable_reset((Object *) opaque, true); +} + +void resettable_class_set_parent_reset_phases(ResettableClass *rc, + ResettableInitPhase init, + ResettableHoldPhase hold, + ResettableExitPhase exit, + ResettablePhases *parent_phases) +{ + *parent_phases = rc->phases; + if (init) { + rc->phases.init = init; + } + if (hold) { + rc->phases.hold = hold; + } + if (exit) { + rc->phases.exit = exit; + } +} + +static const TypeInfo resettable_interface_info = { + .name = TYPE_RESETTABLE, + .parent = TYPE_INTERFACE, + .class_size = sizeof(ResettableClass), +}; + +static void reset_register_types(void) +{ + type_register_static(&resettable_interface_info); +} + +type_init(reset_register_types) diff --git a/hw/core/trace-events b/hw/core/trace-events new file mode 100644 index 0000000000..489d96d445 --- /dev/null +++ b/hw/core/trace-events @@ -0,0 +1,39 @@ +# See docs/devel/tracing.txt for syntax documentation. +# +# This file is processed by the tracetool script during the build. +# +# To add a new trace event: +# +# 1. Choose a name for the trace event. Declare its arguments and format +# string. +# +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() -> +# trace_multiwrite_cb(). The source file must #include "trace.h". +# +# Format of a trace event: +# +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>" +# +# Example: g_malloc(size_t size) "size %zu" +# +# The "disable" keyword will build without the trace event. +# +# The <name> must be a valid as a C function name. +# +# Types should be standard C types. Use void * for pointers because the trace +# system may not have the necessary headers included. +# +# The <format-string> should be a sprintf()-compatible format string. + +# resettable.c +resettable_reset(void *obj, const char *type, int cold) "obj=%p(%s) cold=%d" +resettable_reset_assert(void *obj, const char *type, int cold) "obj=%p(%s) cold=%d" +resettable_reset_deassert(void *obj, const char *type) "obj=%p(%s)" +resettable_reset_deassert_end(void *obj) "obj=%p" +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d" +resettable_phase_init_end(void *obj) "obj=%p" +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)" +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d" +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)" +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32 +resettable_count_underflow(void *obj) "obj=%p" diff --git a/include/hw/resettable.h b/include/hw/resettable.h new file mode 100644 index 0000000000..e617a8e875 --- /dev/null +++ b/include/hw/resettable.h @@ -0,0 +1,126 @@ +#ifndef HW_RESETTABLE_H +#define HW_RESETTABLE_H + +#include "qom/object.h" + +#define TYPE_RESETTABLE "resettable" + +#define RESETTABLE_CLASS(class) \ + OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE) + +/* + * ResettableClass: + * Interface for resettable objects. + * + * The reset operation is divided in several phases each represented by a + * method. + * + * Each Ressetable must maintain a reset counter in its state, 3 methods allows + * to interact with it. + * + * @phases.init: should reset local state only. Takes a bool @cold argument + * specifying whether the reset is cold or warm. It must not do side-effect + * on others objects. + * + * @phases.hold: side-effects action on others objects due to staying in a + * resetting state. + * + * @phases.exit: leave the reset state, may do side-effects action on others + * objects. + * + * @set_cold: Set whether the current reset is cold or warm. Return the + * previous flag value. Return value has no meaning if @get_count returns + * a zero value. + * + * @set_hold_needed: Set hold_needed flag. Return the previous flag value. + * + * @get_count: Get the current reset count + * @increment_count: Increment the reset count, returns the new count + * @decrement_count: decrement the reset count, returns the new count + * + * @foreach_child: Executes a given function on every Resettable child. + * A child is not a QOM child, but a child a reset meaning. + */ +typedef void (*ResettableInitPhase)(Object *obj); +typedef void (*ResettableHoldPhase)(Object *obj); +typedef void (*ResettableExitPhase)(Object *obj); +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; + + struct ResettablePhases { + ResettableInitPhase init; + ResettableHoldPhase hold; + ResettableExitPhase exit; + } phases; + + ResettableSetCold set_cold; + ResettableSetHoldNeeded set_hold_needed; + ResettableGetCount get_count; + ResettableIncrementCount increment_count; + ResettableDecrementCount decrement_count; + ResettableForeachChild foreach_child; +} ResettableClass; +typedef struct ResettablePhases ResettablePhases; + +/** + * resettable_assert_reset: + * Increments the reset count and executes the init and hold phases. + * Each time resettable_assert_reset is called, resettable_deassert_reset + * must eventually be called once. + * It will also impact reset children. + * + * @obj object to reset, must implement Resettable interface. + * @cold boolean indicating the type of reset (cold or warm) + */ +void resettable_assert_reset(Object *obj, bool cold); + +/** + * resettable_deassert_reset: + * Decrements the reset count by one and executes the exit phase if it hits + * zero. + * It will also impact reset children. + * + * @obj object to reset, must implement Resettable interface. + */ +void resettable_deassert_reset(Object *obj); + +/** + * resettable_reset: + * Calling this function is equivalent to call @assert_reset then + * @deassert_reset. + */ +void resettable_reset(Object *obj, bool cold); + +/** + * resettable_reset_warm_fn: + * Helper to call resettable_reset(opaque, false) + */ +void resettable_reset_warm_fn(void *opaque); + +/** + * resettable_reset_cold_fn: + * Helper to call resettable_reset(opaque, true) + */ +void resettable_reset_cold_fn(void *opaque); + +/** + * resettable_class_set_parent_reset_phases: + * + * Save @rc current reset phases into @parent_phases and override @rc phases + * by the given new methods (@init, @hold and @exit). + * Each phase is overriden only if the new one is not NULL allowing to + * override a subset of phases. + */ +void resettable_class_set_parent_reset_phases(ResettableClass *rc, + ResettableInitPhase init, + ResettableHoldPhase hold, + ResettableExitPhase exit, + ResettablePhases *parent_phases); + +#endif
This commit defines an interface allowing multi-phase reset. The phases are INIT, HOLD and EXIT. Each phase has an associated method in the class. The reset of a Resettable is controlled with 2 functions: - resettable_assert_reset which starts the reset operation. - resettable_deassert_reset which ends the reset operation. There is also a `resettable_reset` helper function which does assert then deassert allowing to do a proper reset in one call. In addition, two functions, *resettable_reset_cold_fn* and *resettable_reset_warm_fn*, are defined. They take only an opaque argument (for the Resettable object) and can be used as callbacks. The Resettable interface is "reentrant", _assert_ can be called several times and _deasert_ must be called the same number of times so that the Resettable leave reset state. It is supported by keeping a counter of the current number of _assert_ calls. The counter maintainance is done though 3 methods get/increment/decrement_count. A method set_cold is used to set the cold flag of the reset. Another method set_host_needed is used to ensure hold phase is executed only if required. Reset hierarchy is also supported. Each Resettable may have sub-Resettable objects. When resetting a Resettable, it is propagated to its children using the foreach_child method. When entering reset, init phases are executed parent-to-child order then hold phases are executed child-parent order. When leaving reset, exit phases are executed in child-to-parent order. This will allow to replace current qdev_reset mechanism by this interface without side-effects on reset order. Note: I used an uint32 for the count. This match the type already used in the existing resetting counter in hw/scsi/vmw_pvscsi.c for the PVSCSIState. Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> --- Makefile.objs | 1 + hw/core/Makefile.objs | 1 + hw/core/resettable.c | 220 ++++++++++++++++++++++++++++++++++++++++ hw/core/trace-events | 39 +++++++ include/hw/resettable.h | 126 +++++++++++++++++++++++ 5 files changed, 387 insertions(+) create mode 100644 hw/core/resettable.c create mode 100644 hw/core/trace-events create mode 100644 include/hw/resettable.h