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

[Peter Maydell]

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.

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