| Message ID | 20190729145654.14644-6-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multi-phase reset mechanism | expand |
On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote: > Deprecate old reset apis and make them use the new one while they > are still used somewhere. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > --- > hw/core/qdev.c | 22 +++------------------- > include/hw/qdev-core.h | 28 ++++++++++++++++++++++------ > 2 files changed, 25 insertions(+), 25 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 559ced070d..e9e5f2d5f9 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void (*func)(Object *)) > } > } > > -static int qdev_reset_one(DeviceState *dev, void *opaque) > -{ > - device_legacy_reset(dev); > - > - return 0; > -} > - > -static int qbus_reset_one(BusState *bus, void *opaque) > -{ > - BusClass *bc = BUS_GET_CLASS(bus); > - if (bc->reset) { > - bc->reset(bus); > - } > - return 0; > -} > - > void qdev_reset_all(DeviceState *dev) > { > - qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); > + device_reset(dev, false); > } > > void qdev_reset_all_fn(void *opaque) > @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque) > > void qbus_reset_all(BusState *bus) > { > - qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); > + bus_reset(bus, false); > } > > void qbus_reset_all_fn(void *opaque) > @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > } > } > if (dev->hotplugged) { > - device_legacy_reset(dev); > + device_reset(dev, true); So.. is this change in the device_reset() signature really necessary? Even if there are compelling reasons to handle warm reset in the new API, that doesn't been you need to change device_reset() itself from its established meaning of a cold (i.e. as per power cycle) reset. Warm resets are generally called in rather more specific circumstances (often under guest software direction) so it seems likely that users would want to engage with the new reset API directly. Or we could just create a device_warm_reset() wrapper. That would also avoid the bare boolean parameter, which is not great for readability (you have to look up the signature to have any idea what it means). > } > dev->pending_deleted_event = false; > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index eeb75611c8..1670ae41bb 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -109,6 +109,11 @@ typedef struct DeviceClass { > bool hotpluggable; > > /* callbacks */ > + /* > + * Reset method here is deprecated and replaced by methods in the > + * resettable class interface to implement a multi-phase reset. > + * TODO: remove once every reset callback is unused > + */ > DeviceReset reset; > DeviceRealize realize; > DeviceUnrealize unrealize; > @@ -455,19 +460,22 @@ bool bus_is_resetting(BusState *bus); > */ > bool bus_is_reset_cold(BusState *bus); > > -void qdev_reset_all(DeviceState *dev); > -void qdev_reset_all_fn(void *opaque); > - > /** > - * @qbus_reset_all: > - * @bus: Bus to be reset. > + * qbus/qdev_reset_all: > + * @bus/dev: Bus/Device to be reset. > * > - * Reset @bus and perform a bus-level ("hard") reset of all devices connected > + * Reset @bus/dev and perform a bus-level reset of all devices/buses connected > * to it, including recursive processing of all buses below @bus itself. A > * hard reset means that qbus_reset_all will reset all state of the device. > * For PCI devices, for example, this will include the base address registers > * or configuration space. > + * > + * Theses functions are deprecated, please use device/bus_reset or > + * resettable_reset_* instead > + * TODO: remove them when all occurence are removed > */ > +void qdev_reset_all(DeviceState *dev); > +void qdev_reset_all_fn(void *opaque); > void qbus_reset_all(BusState *bus); > void qbus_reset_all_fn(void *opaque); > > @@ -489,9 +497,17 @@ void qdev_machine_init(void); > * device_legacy_reset: > * > * Reset a single device (by calling the reset method). > + * > + * This function is deprecated, please use device_reset() instead. > + * TODO: remove the function when all occurences are removed. > */ > void device_legacy_reset(DeviceState *dev); > > +/** > + * device_class_set_parent_reset: > + * TODO: remove the function when DeviceClass's reset method > + * is not used anymore. > + */ > void device_class_set_parent_reset(DeviceClass *dc, > DeviceReset dev_reset, > DeviceReset *parent_reset);
On 7/31/19 8:05 AM, David Gibson wrote: > On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote: >> Deprecate old reset apis and make them use the new one while they >> are still used somewhere. >> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >> --- >> hw/core/qdev.c | 22 +++------------------- >> include/hw/qdev-core.h | 28 ++++++++++++++++++++++------ >> 2 files changed, 25 insertions(+), 25 deletions(-) >> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 559ced070d..e9e5f2d5f9 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void (*func)(Object *)) >> } >> } >> >> -static int qdev_reset_one(DeviceState *dev, void *opaque) >> -{ >> - device_legacy_reset(dev); >> - >> - return 0; >> -} >> - >> -static int qbus_reset_one(BusState *bus, void *opaque) >> -{ >> - BusClass *bc = BUS_GET_CLASS(bus); >> - if (bc->reset) { >> - bc->reset(bus); >> - } >> - return 0; >> -} >> - >> void qdev_reset_all(DeviceState *dev) >> { >> - qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); >> + device_reset(dev, false); >> } >> >> void qdev_reset_all_fn(void *opaque) >> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque) >> >> void qbus_reset_all(BusState *bus) >> { >> - qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); >> + bus_reset(bus, false); >> } >> >> void qbus_reset_all_fn(void *opaque) >> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) >> } >> } >> if (dev->hotplugged) { >> - device_legacy_reset(dev); >> + device_reset(dev, true); > > So.. is this change in the device_reset() signature really necessary? > Even if there are compelling reasons to handle warm reset in the new > API, that doesn't been you need to change device_reset() itself from > its established meaning of a cold (i.e. as per power cycle) reset. > Warm resets are generally called in rather more specific circumstances > (often under guest software direction) so it seems likely that users > would want to engage with the new reset API directly. Or we could > just create a device_warm_reset() wrapper. That would also avoid the > bare boolean parameter, which is not great for readability (you have > to look up the signature to have any idea what it means). I've added device_reset_cold/warm wrapper functions to avoid having to pass the boolean parameter. it seems I forgot to use them in qdev.c I suppose, like you said, we could live with + no function with the boolean parameter + device_reset doing cold reset + device_reset_warm (or device_warm_reset) for the warm version Damien
On 7/31/19 11:29 AM, Damien Hedde wrote: > On 7/31/19 8:05 AM, David Gibson wrote: >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote: >>> Deprecate old reset apis and make them use the new one while they >>> are still used somewhere. >>> >>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >>> --- >>> hw/core/qdev.c | 22 +++------------------- >>> include/hw/qdev-core.h | 28 ++++++++++++++++++++++------ >>> 2 files changed, 25 insertions(+), 25 deletions(-) >>> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>> index 559ced070d..e9e5f2d5f9 100644 >>> --- a/hw/core/qdev.c >>> +++ b/hw/core/qdev.c >>> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void (*func)(Object *)) >>> } >>> } >>> >>> -static int qdev_reset_one(DeviceState *dev, void *opaque) >>> -{ >>> - device_legacy_reset(dev); >>> - >>> - return 0; >>> -} >>> - >>> -static int qbus_reset_one(BusState *bus, void *opaque) >>> -{ >>> - BusClass *bc = BUS_GET_CLASS(bus); >>> - if (bc->reset) { >>> - bc->reset(bus); >>> - } >>> - return 0; >>> -} >>> - >>> void qdev_reset_all(DeviceState *dev) >>> { >>> - qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); >>> + device_reset(dev, false); >>> } >>> >>> void qdev_reset_all_fn(void *opaque) >>> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque) >>> >>> void qbus_reset_all(BusState *bus) >>> { >>> - qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); >>> + bus_reset(bus, false); >>> } >>> >>> void qbus_reset_all_fn(void *opaque) >>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) >>> } >>> } >>> if (dev->hotplugged) { >>> - device_legacy_reset(dev); >>> + device_reset(dev, true); >> >> So.. is this change in the device_reset() signature really necessary? >> Even if there are compelling reasons to handle warm reset in the new >> API, that doesn't been you need to change device_reset() itself from >> its established meaning of a cold (i.e. as per power cycle) reset. >> Warm resets are generally called in rather more specific circumstances >> (often under guest software direction) so it seems likely that users >> would want to engage with the new reset API directly. Or we could >> just create a device_warm_reset() wrapper. That would also avoid the >> bare boolean parameter, which is not great for readability (you have >> to look up the signature to have any idea what it means). If the boolean is not meaningful, we can use an enum... > I've added device_reset_cold/warm wrapper functions to avoid having to > pass the boolean parameter. it seems I forgot to use them in qdev.c > I suppose, like you said, we could live with > + no function with the boolean parameter > + device_reset doing cold reset > + device_reset_warm (or device_warm_reset) for the warm version > > Damien >
On Mon, 29 Jul 2019 at 15:58, Damien Hedde <damien.hedde@greensocs.com> wrote: > > Deprecate old reset apis and make them use the new one while they > are still used somewhere. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > --- > hw/core/qdev.c | 22 +++------------------- > include/hw/qdev-core.h | 28 ++++++++++++++++++++++------ > 2 files changed, 25 insertions(+), 25 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 559ced070d..e9e5f2d5f9 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void (*func)(Object *)) > } > } > > -static int qdev_reset_one(DeviceState *dev, void *opaque) > -{ > - device_legacy_reset(dev); > - > - return 0; > -} > - > -static int qbus_reset_one(BusState *bus, void *opaque) > -{ > - BusClass *bc = BUS_GET_CLASS(bus); > - if (bc->reset) { > - bc->reset(bus); > - } > - return 0; > -} > - > void qdev_reset_all(DeviceState *dev) > { > - qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); > + device_reset(dev, false); > } > > void qdev_reset_all_fn(void *opaque) > @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque) > > void qbus_reset_all(BusState *bus) > { > - qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); > + bus_reset(bus, false); > } > > void qbus_reset_all_fn(void *opaque) > @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > } > } > if (dev->hotplugged) { > - device_legacy_reset(dev); > + device_reset(dev, true); > } > dev->pending_deleted_event = false; The other changes here don't change the semantics, but this one does -- previously we were only resetting this specific device and now we're resetting both the device and its children. I think it belongs as its own patch in with the other "remove device_legacy_reset call" patches. > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index eeb75611c8..1670ae41bb 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -109,6 +109,11 @@ typedef struct DeviceClass { > bool hotpluggable; > > /* callbacks */ > + /* > + * Reset method here is deprecated and replaced by methods in the > + * resettable class interface to implement a multi-phase reset. > + * TODO: remove once every reset callback is unused > + */ > DeviceReset reset; > DeviceRealize realize; > DeviceUnrealize unrealize; > @@ -455,19 +460,22 @@ bool bus_is_resetting(BusState *bus); > */ > bool bus_is_reset_cold(BusState *bus); > > -void qdev_reset_all(DeviceState *dev); > -void qdev_reset_all_fn(void *opaque); > - > /** > - * @qbus_reset_all: > - * @bus: Bus to be reset. > + * qbus/qdev_reset_all: > + * @bus/dev: Bus/Device to be reset. > * > - * Reset @bus and perform a bus-level ("hard") reset of all devices connected > + * Reset @bus/dev and perform a bus-level reset of all devices/buses connected > * to it, including recursive processing of all buses below @bus itself. A > * hard reset means that qbus_reset_all will reset all state of the device. > * For PCI devices, for example, this will include the base address registers > * or configuration space. > + * > + * Theses functions are deprecated, please use device/bus_reset or "these" > + * resettable_reset_* instead > + * TODO: remove them when all occurence are removed "occurrences" (two 'r's, plural with 's'), here and below > */ The comment here says that qbus_reset_all() does a "hard" reset, which presumably is equivalent to a 'cold' reset, but the code in our new implementation passes cold = false. > +void qdev_reset_all(DeviceState *dev); > +void qdev_reset_all_fn(void *opaque); > void qbus_reset_all(BusState *bus); > void qbus_reset_all_fn(void *opaque); > > @@ -489,9 +497,17 @@ void qdev_machine_init(void); > * device_legacy_reset: > * > * Reset a single device (by calling the reset method). > + * > + * This function is deprecated, please use device_reset() instead. > + * TODO: remove the function when all occurences are removed. > */ > void device_legacy_reset(DeviceState *dev); > > +/** > + * device_class_set_parent_reset: > + * TODO: remove the function when DeviceClass's reset method > + * is not used anymore. > + */ > void device_class_set_parent_reset(DeviceClass *dc, > DeviceReset dev_reset, > DeviceReset *parent_reset); > -- > 2.22.0 thanks -- PMM
On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote: > On 7/31/19 11:29 AM, Damien Hedde wrote: > > On 7/31/19 8:05 AM, David Gibson wrote: > >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote: > >>> Deprecate old reset apis and make them use the new one while they > >>> are still used somewhere. > >>> > >>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > >>> --- > >>> hw/core/qdev.c | 22 +++------------------- > >>> include/hw/qdev-core.h | 28 ++++++++++++++++++++++------ > >>> 2 files changed, 25 insertions(+), 25 deletions(-) > >>> > >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c > >>> index 559ced070d..e9e5f2d5f9 100644 > >>> --- a/hw/core/qdev.c > >>> +++ b/hw/core/qdev.c > >>> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void (*func)(Object *)) > >>> } > >>> } > >>> > >>> -static int qdev_reset_one(DeviceState *dev, void *opaque) > >>> -{ > >>> - device_legacy_reset(dev); > >>> - > >>> - return 0; > >>> -} > >>> - > >>> -static int qbus_reset_one(BusState *bus, void *opaque) > >>> -{ > >>> - BusClass *bc = BUS_GET_CLASS(bus); > >>> - if (bc->reset) { > >>> - bc->reset(bus); > >>> - } > >>> - return 0; > >>> -} > >>> - > >>> void qdev_reset_all(DeviceState *dev) > >>> { > >>> - qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); > >>> + device_reset(dev, false); > >>> } > >>> > >>> void qdev_reset_all_fn(void *opaque) > >>> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque) > >>> > >>> void qbus_reset_all(BusState *bus) > >>> { > >>> - qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); > >>> + bus_reset(bus, false); > >>> } > >>> > >>> void qbus_reset_all_fn(void *opaque) > >>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > >>> } > >>> } > >>> if (dev->hotplugged) { > >>> - device_legacy_reset(dev); > >>> + device_reset(dev, true); > >> > >> So.. is this change in the device_reset() signature really necessary? > >> Even if there are compelling reasons to handle warm reset in the new > >> API, that doesn't been you need to change device_reset() itself from > >> its established meaning of a cold (i.e. as per power cycle) reset. > >> Warm resets are generally called in rather more specific circumstances > >> (often under guest software direction) so it seems likely that users > >> would want to engage with the new reset API directly. Or we could > >> just create a device_warm_reset() wrapper. That would also avoid the > >> bare boolean parameter, which is not great for readability (you have > >> to look up the signature to have any idea what it means). > > If the boolean is not meaningful, we can use an enum... That's certainly better, but I'm not seeing a compelling reason not to have separate function names. It's just as clear and means less churn. > > > I've added device_reset_cold/warm wrapper functions to avoid having to > > pass the boolean parameter. it seems I forgot to use them in qdev.c > > I suppose, like you said, we could live with > > + no function with the boolean parameter > > + device_reset doing cold reset > > + device_reset_warm (or device_warm_reset) for the warm version > > > > Damien > > >
On Wed, Jul 31, 2019 at 11:29:36AM +0200, Damien Hedde wrote: > > > On 7/31/19 8:05 AM, David Gibson wrote: > > On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote: > >> Deprecate old reset apis and make them use the new one while they > >> are still used somewhere. > >> > >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > >> --- > >> hw/core/qdev.c | 22 +++------------------- > >> include/hw/qdev-core.h | 28 ++++++++++++++++++++++------ > >> 2 files changed, 25 insertions(+), 25 deletions(-) > >> > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c > >> index 559ced070d..e9e5f2d5f9 100644 > >> --- a/hw/core/qdev.c > >> +++ b/hw/core/qdev.c > >> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void (*func)(Object *)) > >> } > >> } > >> > >> -static int qdev_reset_one(DeviceState *dev, void *opaque) > >> -{ > >> - device_legacy_reset(dev); > >> - > >> - return 0; > >> -} > >> - > >> -static int qbus_reset_one(BusState *bus, void *opaque) > >> -{ > >> - BusClass *bc = BUS_GET_CLASS(bus); > >> - if (bc->reset) { > >> - bc->reset(bus); > >> - } > >> - return 0; > >> -} > >> - > >> void qdev_reset_all(DeviceState *dev) > >> { > >> - qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); > >> + device_reset(dev, false); > >> } > >> > >> void qdev_reset_all_fn(void *opaque) > >> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque) > >> > >> void qbus_reset_all(BusState *bus) > >> { > >> - qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); > >> + bus_reset(bus, false); > >> } > >> > >> void qbus_reset_all_fn(void *opaque) > >> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > >> } > >> } > >> if (dev->hotplugged) { > >> - device_legacy_reset(dev); > >> + device_reset(dev, true); > > > > So.. is this change in the device_reset() signature really necessary? > > Even if there are compelling reasons to handle warm reset in the new > > API, that doesn't been you need to change device_reset() itself from > > its established meaning of a cold (i.e. as per power cycle) reset. > > Warm resets are generally called in rather more specific circumstances > > (often under guest software direction) so it seems likely that users > > would want to engage with the new reset API directly. Or we could > > just create a device_warm_reset() wrapper. That would also avoid the > > bare boolean parameter, which is not great for readability (you have > > to look up the signature to have any idea what it means). > > I've added device_reset_cold/warm wrapper functions to avoid having to > pass the boolean parameter. it seems I forgot to use them in qdev.c > I suppose, like you said, we could live with > + no function with the boolean parameter > + device_reset doing cold reset > + device_reset_warm (or device_warm_reset) for the warm version Ok, good. I'm afraid the whole series still makes me pretty uncomfortable, though, since the whole "warm reset" concept still seems way to vague to me.
On Fri, 9 Aug 2019 at 01:10, David Gibson <david@gibson.dropbear.id.au> wrote: > > On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote: > > On 7/31/19 11:29 AM, Damien Hedde wrote: > > > On 7/31/19 8:05 AM, David Gibson wrote: > > >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote: > > >>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > > >>> } > > >>> } > > >>> if (dev->hotplugged) { > > >>> - device_legacy_reset(dev); > > >>> + device_reset(dev, true); > > >> > > >> So.. is this change in the device_reset() signature really necessary? > > >> Even if there are compelling reasons to handle warm reset in the new > > >> API, that doesn't been you need to change device_reset() itself from > > >> its established meaning of a cold (i.e. as per power cycle) reset. So, I don't think that actually is the established meaning of device_reset(). I think our current semantics for this function are "reset of some sort, probably cold, but in practice called in lots of places which really wanted some other kind of reset, because our current reset semantics are not well-defined". For instance in s390-pci-inst.c the handling of CLP_SET_DISABLE_PCI_FN calls device_reset() on a device. I bet that's not really intentionally trying to model "we powered it off and on again". hw/scsi/vmw_pvscsi.c uses device_reset() in its handling of the guest "reset the SCSI bus" command. I know that isn't literally powering off the SCSI disks and powering them on again. The advantage of an actual API change here is that it means we go and look at all the call sites and find out what the semantics they actually wanted were, and hopefully that then feeds into the design of the new API and we make sure we can handle those semantics in a non-confused way. > > >> Warm resets are generally called in rather more specific circumstances > > >> (often under guest software direction) so it seems likely that users > > >> would want to engage with the new reset API directly. Or we could > > >> just create a device_warm_reset() wrapper. That would also avoid the > > >> bare boolean parameter, which is not great for readability (you have > > >> to look up the signature to have any idea what it means). > > > > If the boolean is not meaningful, we can use an enum... > > That's certainly better, but I'm not seeing a compelling reason not to > have separate function names. It's just as clear and means less churn. One advantage of an enum is that we have an extendable API, so we could say something like "all devices support reset types 'cold' and 'warm', but individual devices or families of devices can also support other kinds of reset". So the relevant s390 devices could directly support the kinds of reset currently enumerated by the enum s390_reset, and then if you know you're dealing with an s390 device you can ask it to reset with the right semantics rather than fudging it with one of the generic ones. Or devices with "if I'm reset by the guest writing to a register then I reset less stuff than a reset via external reset line" have a way to model that. thanks -- PMM
>>> So.. is this change in the device_reset() signature really necessary? >>> Even if there are compelling reasons to handle warm reset in the new >>> API, that doesn't been you need to change device_reset() itself from >>> its established meaning of a cold (i.e. as per power cycle) reset. >>> Warm resets are generally called in rather more specific circumstances >>> (often under guest software direction) so it seems likely that users >>> would want to engage with the new reset API directly. Or we could >>> just create a device_warm_reset() wrapper. That would also avoid the >>> bare boolean parameter, which is not great for readability (you have >>> to look up the signature to have any idea what it means). >> >> I've added device_reset_cold/warm wrapper functions to avoid having to >> pass the boolean parameter. it seems I forgot to use them in qdev.c >> I suppose, like you said, we could live with >> + no function with the boolean parameter >> + device_reset doing cold reset >> + device_reset_warm (or device_warm_reset) for the warm version > > Ok, good. > > I'm afraid the whole series still makes me pretty uncomfortable, > though, since the whole "warm reset" concept still seems way to vague > to me. Isn't the reset after the CAS negotiation sequence between the hypervisor and the pseries machine some sort of warm reset driven by SW ? C.
On Fri, Aug 09, 2019 at 12:08:43PM +0100, Peter Maydell wrote: > On Fri, 9 Aug 2019 at 01:10, David Gibson <david@gibson.dropbear.id.au> wrote: > > > > On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote: > > > On 7/31/19 11:29 AM, Damien Hedde wrote: > > > > On 7/31/19 8:05 AM, David Gibson wrote: > > > >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote: > > > >>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > > > >>> } > > > >>> } > > > >>> if (dev->hotplugged) { > > > >>> - device_legacy_reset(dev); > > > >>> + device_reset(dev, true); > > > >> > > > >> So.. is this change in the device_reset() signature really necessary? > > > >> Even if there are compelling reasons to handle warm reset in the new > > > >> API, that doesn't been you need to change device_reset() itself from > > > >> its established meaning of a cold (i.e. as per power cycle) reset. > > So, I don't think that actually is the established meaning of > device_reset(). I think our current semantics for this function are > "reset of some sort, probably cold, but in practice called in > lots of places which really wanted some other kind of reset, > because our current reset semantics are not well-defined". > > For instance in s390-pci-inst.c the handling of CLP_SET_DISABLE_PCI_FN > calls device_reset() on a device. I bet that's not really intentionally > trying to model "we powered it off and on again". > hw/scsi/vmw_pvscsi.c uses device_reset() in its handling of > the guest "reset the SCSI bus" command. I know that isn't literally > powering off the SCSI disks and powering them on again. > > The advantage of an actual API change here is that it means we go > and look at all the call sites and find out what the semantics > they actually wanted were, and hopefully that then feeds into the > design of the new API and we make sure we can handle those > semantics in a non-confused way. That's a fair point. > > > >> Warm resets are generally called in rather more specific circumstances > > > >> (often under guest software direction) so it seems likely that users > > > >> would want to engage with the new reset API directly. Or we could > > > >> just create a device_warm_reset() wrapper. That would also avoid the > > > >> bare boolean parameter, which is not great for readability (you have > > > >> to look up the signature to have any idea what it means). > > > > > > If the boolean is not meaningful, we can use an enum... > > > > That's certainly better, but I'm not seeing a compelling reason not to > > have separate function names. It's just as clear and means less churn. > > One advantage of an enum is that we have an extendable API, > so we could say something like "all devices support reset types > 'cold' and 'warm', but individual devices or families of devices > can also support other kinds of reset". So the relevant s390 > devices could directly support the kinds of reset currently > enumerated by the enum s390_reset, and then if you know you're > dealing with an s390 device you can ask it to reset with the > right semantics rather than fudging it with one of the generic ones. > Or devices with "if I'm reset by the guest writing to a register then > I reset less stuff than a reset via external reset line" have a > way to model that. Ok, I can see the value in that. I guess the way I'd prefer to approach it though, is to start out with just a single-value enum for (roughly) a cold reset. Then when we find a subset of devices for which we can consistently define a warm reset of some type, we add an enum value for it. I guess we'd also need some way of introspecting what reset types are supported by a device.
On Fri, Aug 09, 2019 at 01:39:46PM +0200, Cédric Le Goater wrote: > > >>> So.. is this change in the device_reset() signature really necessary? > >>> Even if there are compelling reasons to handle warm reset in the new > >>> API, that doesn't been you need to change device_reset() itself from > >>> its established meaning of a cold (i.e. as per power cycle) reset. > >>> Warm resets are generally called in rather more specific circumstances > >>> (often under guest software direction) so it seems likely that users > >>> would want to engage with the new reset API directly. Or we could > >>> just create a device_warm_reset() wrapper. That would also avoid the > >>> bare boolean parameter, which is not great for readability (you have > >>> to look up the signature to have any idea what it means). > >> > >> I've added device_reset_cold/warm wrapper functions to avoid having to > >> pass the boolean parameter. it seems I forgot to use them in qdev.c > >> I suppose, like you said, we could live with > >> + no function with the boolean parameter > >> + device_reset doing cold reset > >> + device_reset_warm (or device_warm_reset) for the warm version > > > > Ok, good. > > > > I'm afraid the whole series still makes me pretty uncomfortable, > > though, since the whole "warm reset" concept still seems way to vague > > to me. > > Isn't the reset after the CAS negotiation sequence between the hypervisor > and the pseries machine some sort of warm reset driven by SW ? Yes.. and? The fact that something as messy as CAS can come under the category of warm reset only re-inforces that what a warm reset is isn't really well defined. [That said, in the case of CAS, I'd really like it if we can change things to avoid the pseudo-reset and just rewrite the dt instead. The sorta-reboot causes us problems with -no-reboot and with disabling the SLOF autoboot flag]
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 559ced070d..e9e5f2d5f9 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void (*func)(Object *)) } } -static int qdev_reset_one(DeviceState *dev, void *opaque) -{ - device_legacy_reset(dev); - - return 0; -} - -static int qbus_reset_one(BusState *bus, void *opaque) -{ - BusClass *bc = BUS_GET_CLASS(bus); - if (bc->reset) { - bc->reset(bus); - } - return 0; -} - void qdev_reset_all(DeviceState *dev) { - qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); + device_reset(dev, false); } void qdev_reset_all_fn(void *opaque) @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque) void qbus_reset_all(BusState *bus) { - qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL); + bus_reset(bus, false); } void qbus_reset_all_fn(void *opaque) @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } } if (dev->hotplugged) { - device_legacy_reset(dev); + device_reset(dev, true); } dev->pending_deleted_event = false; diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index eeb75611c8..1670ae41bb 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -109,6 +109,11 @@ typedef struct DeviceClass { bool hotpluggable; /* callbacks */ + /* + * Reset method here is deprecated and replaced by methods in the + * resettable class interface to implement a multi-phase reset. + * TODO: remove once every reset callback is unused + */ DeviceReset reset; DeviceRealize realize; DeviceUnrealize unrealize; @@ -455,19 +460,22 @@ bool bus_is_resetting(BusState *bus); */ bool bus_is_reset_cold(BusState *bus); -void qdev_reset_all(DeviceState *dev); -void qdev_reset_all_fn(void *opaque); - /** - * @qbus_reset_all: - * @bus: Bus to be reset. + * qbus/qdev_reset_all: + * @bus/dev: Bus/Device to be reset. * - * Reset @bus and perform a bus-level ("hard") reset of all devices connected + * Reset @bus/dev and perform a bus-level reset of all devices/buses connected * to it, including recursive processing of all buses below @bus itself. A * hard reset means that qbus_reset_all will reset all state of the device. * For PCI devices, for example, this will include the base address registers * or configuration space. + * + * Theses functions are deprecated, please use device/bus_reset or + * resettable_reset_* instead + * TODO: remove them when all occurence are removed */ +void qdev_reset_all(DeviceState *dev); +void qdev_reset_all_fn(void *opaque); void qbus_reset_all(BusState *bus); void qbus_reset_all_fn(void *opaque); @@ -489,9 +497,17 @@ void qdev_machine_init(void); * device_legacy_reset: * * Reset a single device (by calling the reset method). + * + * This function is deprecated, please use device_reset() instead. + * TODO: remove the function when all occurences are removed. */ void device_legacy_reset(DeviceState *dev); +/** + * device_class_set_parent_reset: + * TODO: remove the function when DeviceClass's reset method + * is not used anymore. + */ void device_class_set_parent_reset(DeviceClass *dc, DeviceReset dev_reset, DeviceReset *parent_reset);
Deprecate old reset apis and make them use the new one while they are still used somewhere. Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> --- hw/core/qdev.c | 22 +++------------------- include/hw/qdev-core.h | 28 ++++++++++++++++++++++------ 2 files changed, 25 insertions(+), 25 deletions(-)