* [PATCH 1/3] hw/intc/spapr_xive: Use device_cold_reset() instead of device_legacy_reset()
2021-05-03 15:18 [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset() Peter Maydell
@ 2021-05-03 15:18 ` Peter Maydell
2021-05-04 0:32 ` David Gibson
2021-05-03 15:18 ` [PATCH 2/3] hw/ppc/spapr_vio: Reset TCE table object with device_cold_reset() Peter Maydell
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2021-05-03 15:18 UTC (permalink / raw)
To: qemu-devel; +Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, David Gibson
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hw/intc/spapr_xive: Use device_cold_reset() instead of device_legacy_reset()
2021-05-03 15:18 ` [PATCH 1/3] hw/intc/spapr_xive: Use device_cold_reset() instead of device_legacy_reset() Peter Maydell
@ 2021-05-04 0:32 ` David Gibson
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2021-05-04 0:32 UTC (permalink / raw)
To: Peter Maydell; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] hw/ppc/spapr_vio: Reset TCE table object with device_cold_reset()
2021-05-03 15:18 [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset() Peter Maydell
2021-05-03 15:18 ` [PATCH 1/3] hw/intc/spapr_xive: Use device_cold_reset() instead of device_legacy_reset() Peter Maydell
@ 2021-05-03 15:18 ` Peter Maydell
2021-05-04 0:36 ` David Gibson
2021-05-03 15:18 ` [PATCH 3/3] hw/ppc/pnv_psi: Use device_cold_reset() instead of device_legacy_reset() Peter Maydell
2021-05-03 16:55 ` [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset() Philippe Mathieu-Daudé
3 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2021-05-03 15:18 UTC (permalink / raw)
To: qemu-devel; +Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, David Gibson
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] hw/ppc/spapr_vio: Reset TCE table object with device_cold_reset()
2021-05-03 15:18 ` [PATCH 2/3] hw/ppc/spapr_vio: Reset TCE table object with device_cold_reset() Peter Maydell
@ 2021-05-04 0:36 ` David Gibson
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2021-05-04 0:36 UTC (permalink / raw)
To: Peter Maydell; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] hw/ppc/pnv_psi: Use device_cold_reset() instead of device_legacy_reset()
2021-05-03 15:18 [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset() Peter Maydell
2021-05-03 15:18 ` [PATCH 1/3] hw/intc/spapr_xive: Use device_cold_reset() instead of device_legacy_reset() Peter Maydell
2021-05-03 15:18 ` [PATCH 2/3] hw/ppc/spapr_vio: Reset TCE table object with device_cold_reset() Peter Maydell
@ 2021-05-03 15:18 ` Peter Maydell
2021-05-04 0:37 ` David Gibson
2021-05-03 16:55 ` [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset() Philippe Mathieu-Daudé
3 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2021-05-03 15:18 UTC (permalink / raw)
To: qemu-devel; +Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, David Gibson
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] hw/ppc/pnv_psi: Use device_cold_reset() instead of device_legacy_reset()
2021-05-03 15:18 ` [PATCH 3/3] hw/ppc/pnv_psi: Use device_cold_reset() instead of device_legacy_reset() Peter Maydell
@ 2021-05-04 0:37 ` David Gibson
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2021-05-04 0:37 UTC (permalink / raw)
To: Peter Maydell; +Cc: Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset()
2021-05-03 15:18 [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset() Peter Maydell
` (2 preceding siblings ...)
2021-05-03 15:18 ` [PATCH 3/3] hw/ppc/pnv_psi: Use device_cold_reset() instead of device_legacy_reset() Peter Maydell
@ 2021-05-03 16:55 ` Philippe Mathieu-Daudé
2021-05-03 17:15 ` Peter Maydell
3 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-03 16:55 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, Eduardo Habkost, Markus Armbruster
Cc: David Gibson, qemu-ppc, Cédric Le Goater, Greg Kurz
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset()
2021-05-03 16:55 ` [PATCH 0/3] ppc: Convert (mostly) from device_legacy_reset() to device_cold_reset() Philippe Mathieu-Daudé
@ 2021-05-03 17:15 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2021-05-03 17:15 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Eduardo Habkost, QEMU Developers, Markus Armbruster, Greg Kurz,
qemu-ppc, Cédric Le Goater, David Gibson
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
^ permalink raw reply [flat|nested] 9+ messages in thread