[PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset()

Peter Maydell posted 3 patches 2 weeks ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210503151849.8766-1-peter.maydell@linaro.org
Maintainers: Greg Kurz <groug@kaod.org>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>
hw/intc/spapr_xive.c | 2 +-
hw/ppc/pnv_psi.c     | 4 ++--
hw/ppc/spapr_vio.c   | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)

[PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset()

Posted by Peter Maydell 2 weeks ago
The old function device_legacy_reset() (which was originally
device_reset() and got renamed when 3-phase-reset landed) is
deprecated, because it has slightly odd semantics -- it resets the
device itself, but (unlike when a device is reset as part of system
reset) not any qbus it owns (and devices attached to the qbus).  The
replacement is device_cold_reset(), which resets the device and its
bus (if any).

For a device with child bus, the two functions are identical; this
patchset changes the PPC code which uses device_legacy_reset() on
devices which have no qbus to use device_cold_reset() instead; this
should have no functionally visible difference.

There is one other use of device_legacy_reset() in PPC code which I
didn't change: in hw/ppc/spapr_pci.c:spapr_phb_children_reset().  I
couldn't figure out what the children being reset here are and if
they might own buses.  I suspect that even if they do own buses the
right thing would be to change to device_cold_reset(), but I stuck to
only the changes I felt reasonably sure were definitely
no-behaviour-change.

NB: tested with 'make check' and 'make check-acceptance' only.

thanks
-- PMM

Peter Maydell (3):
  hw/intc/spapr_xive: Use device_cold_reset() instead of
    device_legacy_reset()
  hw/ppc/spapr_vio: Reset TCE table object with device_cold_reset()
  hw/ppc/pnv_psi: Use device_cold_reset() instead of
    device_legacy_reset()

 hw/intc/spapr_xive.c | 2 +-
 hw/ppc/pnv_psi.c     | 4 ++--
 hw/ppc/spapr_vio.c   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.20.1


Re: [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset()

Posted by Philippe Mathieu-Daudé 2 weeks ago
Hi Peter,

+Eduardo/Markus

On 5/3/21 5:18 PM, Peter Maydell wrote:
> The old function device_legacy_reset() (which was originally
> device_reset() and got renamed when 3-phase-reset landed) is
> deprecated, because it has slightly odd semantics -- it resets the
> device itself, but (unlike when a device is reset as part of system
> reset) not any qbus it owns (and devices attached to the qbus).  The
> replacement is device_cold_reset(), which resets the device and its
> bus (if any).
> 
> For a device with child bus, the two functions are identical; this
> patchset changes the PPC code which uses device_legacy_reset() on
> devices which have no qbus to use device_cold_reset() instead; this
> should have no functionally visible difference.

So IIUC we should be able to add this check?

-- >8 --
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a9..4e03f964a42 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1121,6 +1122,7 @@ void device_legacy_reset(DeviceState *dev)
     DeviceClass *klass = DEVICE_GET_CLASS(dev);

     trace_qdev_reset(dev, object_get_typename(OBJECT(dev)));
+    assert(DEVICE_GET_CLASS(dev)->bus_type);
     if (klass->reset) {
         klass->reset(dev);
     }
---

> 
> There is one other use of device_legacy_reset() in PPC code which I
> didn't change: in hw/ppc/spapr_pci.c:spapr_phb_children_reset().  I
> couldn't figure out what the children being reset here are and if
> they might own buses.  I suspect that even if they do own buses the
> right thing would be to change to device_cold_reset(), but I stuck to
> only the changes I felt reasonably sure were definitely
> no-behaviour-change.
> 
> NB: tested with 'make check' and 'make check-acceptance' only.
> 
> thanks
> -- PMM


Re: [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset()

Posted by Peter Maydell 2 weeks ago
On Mon, 3 May 2021 at 17:55, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Peter,
>
> +Eduardo/Markus
>
> On 5/3/21 5:18 PM, Peter Maydell wrote:
> > The old function device_legacy_reset() (which was originally
> > device_reset() and got renamed when 3-phase-reset landed) is
> > deprecated, because it has slightly odd semantics -- it resets the
> > device itself, but (unlike when a device is reset as part of system
> > reset) not any qbus it owns (and devices attached to the qbus).  The
> > replacement is device_cold_reset(), which resets the device and its
> > bus (if any).
> >
> > For a device with child bus, the two functions are identical; this
> > patchset changes the PPC code which uses device_legacy_reset() on
> > devices which have no qbus to use device_cold_reset() instead; this
> > should have no functionally visible difference.
>
> So IIUC we should be able to add this check?
>
> -- >8 --
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cefc5eaa0a9..4e03f964a42 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1121,6 +1122,7 @@ void device_legacy_reset(DeviceState *dev)
>      DeviceClass *klass = DEVICE_GET_CLASS(dev);
>
>      trace_qdev_reset(dev, object_get_typename(OBJECT(dev)));
> +    assert(DEVICE_GET_CLASS(dev)->bus_type);
>      if (klass->reset) {
>          klass->reset(dev);
>      }
> ---

No, because the codepath starting at qdev_reset_all() which
resets the whole qbus-and-devices tree calls device_legacy_reset()
to reset a single device.

There are also probably places where we call device_legacy_reset()
on devices which do have qbuses, where we need to figure out
what to do (eg if they are also manually resetting the qbuses
we could switch to using device_cold_reset(), or perhaps they
aren't resetting the qbuses and this is technically a bug.
Either way, those callsites are harder to convert.

thanks
-- PMM