Re: [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus

[David Gibson]

On Fri, Aug 09, 2019 at 12:08:43PM +0100, Peter Maydell wrote:
> On Fri, 9 Aug 2019 at 01:10, David Gibson <address@hidden> 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

signature.asc
Description: PGP signature