On Fri, Aug 09, 2019 at 12:08:43PM +0100, Peter Maydell wrote: > On Fri, 9 Aug 2019 at 01:10, David Gibson 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. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson