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

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

[Peter Maydell]

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

[PATCH 1/3] hw/intc/spapr_xive: Use device_cold_reset() instead of device_legacy_reset()

[Peter Maydell]

The h_int_reset() function resets the XIVE interrupt controller via
device_legacy_reset().  We know that the interrupt controller does
not have a qbus of its own, so the new device_cold_reset() function
(which resets both the device and its child buses) is equivalent here
to device_legacy_reset() and we can just switch to the new API.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/spapr_xive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 801bc193416..89cfa018f59 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -1798,7 +1798,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
         return H_PARAMETER;
     }
 
-    device_legacy_reset(DEVICE(xive));
+    device_cold_reset(DEVICE(xive));
 
     if (spapr_xive_in_kernel(xive)) {
         Error *local_err = NULL;
-- 
2.20.1

Re: [PATCH 1/3] hw/intc/spapr_xive: Use device_cold_reset() instead of device_legacy_reset()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On Mon, May 03, 2021 at 04:18:47PM +0100, Peter Maydell wrote:
> The h_int_reset() function resets the XIVE interrupt controller via
> device_legacy_reset().  We know that the interrupt controller does
> not have a qbus of its own, so the new device_cold_reset() function
> (which resets both the device and its child buses) is equivalent here
> to device_legacy_reset() and we can just switch to the new API.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Applied to ppc-for-6.1.

> ---
>  hw/intc/spapr_xive.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 801bc193416..89cfa018f59 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -1798,7 +1798,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
>          return H_PARAMETER;
>      }
>  
> -    device_legacy_reset(DEVICE(xive));
> +    device_cold_reset(DEVICE(xive));
>  
>      if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] hw/ppc/spapr_vio: Reset TCE table object with device_cold_reset()

[Peter Maydell]

The spapr_vio_quiesce_one() function resets the TCE table object
(TYPE_SPAPR_TCE_TABLE) via device_legacy_reset().  We know that
objects of that type do not have a qbus of their own, so the new
device_cold_reset() function (which resets both the device and its
child buses) is equivalent here to device_legacy_reset() and we can
just switch to the new API.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/spapr_vio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index ef06e0362c8..b59452bcd62 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -310,7 +310,7 @@ int spapr_vio_send_crq(SpaprVioDevice *dev, uint8_t *crq)
 static void spapr_vio_quiesce_one(SpaprVioDevice *dev)
 {
     if (dev->tcet) {
-        device_legacy_reset(DEVICE(dev->tcet));
+        device_cold_reset(DEVICE(dev->tcet));
     }
     free_crq(dev);
 }
-- 
2.20.1

Re: [PATCH 2/3] hw/ppc/spapr_vio: Reset TCE table object with device_cold_reset()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

On Mon, May 03, 2021 at 04:18:48PM +0100, Peter Maydell wrote:
> The spapr_vio_quiesce_one() function resets the TCE table object
> (TYPE_SPAPR_TCE_TABLE) via device_legacy_reset().  We know that
> objects of that type do not have a qbus of their own, so the new
> device_cold_reset() function (which resets both the device and its
> child buses) is equivalent here to device_legacy_reset() and we can
> just switch to the new API.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Applied to ppc-for-6.1.

Actually, I strongly suspect we could just change rtas_quiesce() to
cold reset the entire VIO bus, but we can look at that as a followup
after checking  bit more closely.

> ---
>  hw/ppc/spapr_vio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index ef06e0362c8..b59452bcd62 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -310,7 +310,7 @@ int spapr_vio_send_crq(SpaprVioDevice *dev, uint8_t *crq)
>  static void spapr_vio_quiesce_one(SpaprVioDevice *dev)
>  {
>      if (dev->tcet) {
> -        device_legacy_reset(DEVICE(dev->tcet));
> +        device_cold_reset(DEVICE(dev->tcet));
>      }
>      free_crq(dev);
>  }

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] hw/ppc/pnv_psi: Use device_cold_reset() instead of device_legacy_reset()

[Peter Maydell]

The pnv_psi.c code uses device_legacy_reset() for two purposes:
 * to reset itself from its qemu_register_reset() handler
 * to reset a XiveSource object it has

Neither it nor the XiveSource have any qbuses, so the new
device_cold_reset() function (which resets both the device and its
child buses) is equivalent here to device_legacy_reset() and we can
just switch to the new API.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/pnv_psi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 3e868c8c8da..292b373f93f 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -466,7 +466,7 @@ static void pnv_psi_reset(DeviceState *dev)
 
 static void pnv_psi_reset_handler(void *dev)
 {
-    device_legacy_reset(DEVICE(dev));
+    device_cold_reset(DEVICE(dev));
 }
 
 static void pnv_psi_realize(DeviceState *dev, Error **errp)
@@ -710,7 +710,7 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
         break;
     case PSIHB9_INTERRUPT_CONTROL:
         if (val & PSIHB9_IRQ_RESET) {
-            device_legacy_reset(DEVICE(&psi9->source));
+            device_cold_reset(DEVICE(&psi9->source));
         }
         psi->regs[reg] = val;
         break;
-- 
2.20.1

Re: [PATCH 3/3] hw/ppc/pnv_psi: Use device_cold_reset() instead of device_legacy_reset()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

On Mon, May 03, 2021 at 04:18:49PM +0100, Peter Maydell wrote:
> The pnv_psi.c code uses device_legacy_reset() for two purposes:
>  * to reset itself from its qemu_register_reset() handler
>  * to reset a XiveSource object it has
> 
> Neither it nor the XiveSource have any qbuses, so the new
> device_cold_reset() function (which resets both the device and its
> child buses) is equivalent here to device_legacy_reset() and we can
> just switch to the new API.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Applied to ppc-for-6.1.

> ---
>  hw/ppc/pnv_psi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 3e868c8c8da..292b373f93f 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -466,7 +466,7 @@ static void pnv_psi_reset(DeviceState *dev)
>  
>  static void pnv_psi_reset_handler(void *dev)
>  {
> -    device_legacy_reset(DEVICE(dev));
> +    device_cold_reset(DEVICE(dev));
>  }
>  
>  static void pnv_psi_realize(DeviceState *dev, Error **errp)
> @@ -710,7 +710,7 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>          break;
>      case PSIHB9_INTERRUPT_CONTROL:
>          if (val & PSIHB9_IRQ_RESET) {
> -            device_legacy_reset(DEVICE(&psi9->source));
> +            device_cold_reset(DEVICE(&psi9->source));
>          }
>          psi->regs[reg] = val;
>          break;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Philippe Mathieu-Daudé]

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

[Peter Maydell]

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