[PATCH v2 0/3] hw: Fix reset of bus-less devices

[PATCH v2 0/3] hw: Fix reset of bus-less devices

[Philippe Mathieu-Daudé]

Hi,

This series is the result of a code audit of the DeviceClass::reset()
method uses, having Markus following explanation in mind [1]:

  "Propagating reset from the root of the qtree to the leaves
  won't reach a bus-less device, because the qtree contains
  only the devices that plug into a qbus."

Which is a resumed of what Peter said earlier in the thread [2].

Since v1:
- removed spapr_iommu patch (2 different reset is intentional)
- removed microdrive patch (pcmcia cards should plug into a bus)
- use device_cold_reset() instead of device_legacy_reset
- use ResettableClass instead of deprecated DeviceClass::reset

(patch changed -> not carrying David A-b tag)

Philippe Mathieu-Daudé (3):
  hw/block/nand: Register machine reset handler
  hw/pci-host/raven: Manually reset the OR_IRQ device
  hw/arm/armsse: Manually reset the OR_IRQ devices

 hw/arm/armsse.c    |  4 ++++
 hw/block/nand.c    | 14 ++++++++++++++
 hw/pci-host/prep.c | 11 +++++++++++
 3 files changed, 29 insertions(+)

-- 
2.26.3

[PATCH v2 1/3] hw/block/nand: Register machine reset handler

[Philippe Mathieu-Daudé]

The TYPE_NAND device is bus-less, thus isn't reset automatically.
Register a reset handler to get reset with the machine.

Fixed: 7426aa72c36 ("nand: Don't inherit from Sysbus")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/nand.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 8bc80e35144..8ec656659f5 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -24,6 +24,7 @@
 #include "hw/qdev-properties-system.h"
 #include "hw/block/flash.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/reset.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
@@ -364,6 +365,11 @@ static const VMStateDescription vmstate_nand = {
     }
 };
 
+static void nand_reset_handler(void *dev)
+{
+    device_cold_reset(DEVICE(dev));
+}
+
 static void nand_realize(DeviceState *dev, Error **errp)
 {
     int pagesize;
@@ -423,6 +429,13 @@ static void nand_realize(DeviceState *dev, Error **errp)
     }
     /* Give s->ioaddr a sane value in case we save state before it is used. */
     s->ioaddr = s->io;
+
+    qemu_register_reset(nand_reset_handler, dev);
+}
+
+static void nand_unrealize(DeviceState *dev)
+{
+    qemu_unregister_reset(nand_reset_handler, dev);
 }
 
 static Property nand_properties[] = {
@@ -437,6 +450,7 @@ static void nand_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = nand_realize;
+    dc->unrealize = nand_unrealize;
     dc->reset = nand_reset;
     dc->vmsd = &vmstate_nand;
     device_class_set_props(dc, nand_properties);
-- 
2.26.3

Re: [PATCH v2 1/3] hw/block/nand: Register machine reset handler

[Peter Maydell]

On Sun, 2 May 2021 at 21:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The TYPE_NAND device is bus-less, thus isn't reset automatically.
> Register a reset handler to get reset with the machine.
>
> Fixed: 7426aa72c36 ("nand: Don't inherit from Sysbus")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

Personally I would just revert 7426aa72c36 rather than introduce
a new use of qemu_register_reset()...

thanks
-- PMM

Re: [PATCH v2 1/3] hw/block/nand: Register machine reset handler

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Sun, 2 May 2021 at 21:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The TYPE_NAND device is bus-less, thus isn't reset automatically.
>> Register a reset handler to get reset with the machine.
>>
>> Fixed: 7426aa72c36 ("nand: Don't inherit from Sysbus")
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>
> Personally I would just revert 7426aa72c36 rather than introduce
> a new use of qemu_register_reset()...

I think this depends on what you dislike more, qemu_register_reset() or
sysbus.  Neither is deprecated, both have more than a hundred users.
One more user doesn't matter all that much for either.

I happen to dislike sysbus more, but my opinion on this is yet another
thin that doesn't matter all that much :)

[PATCH v2 2/3] hw/pci-host/raven: Manually reset the OR_IRQ device

[Philippe Mathieu-Daudé]

The OR_IRQ device is bus-less, thus isn't reset automatically.
Add the raven_pcihost_reset() handler to manually reset the OR IRQ.

Fixes: f40b83a4e31 ("40p: use OR gate to wire up raven PCI interrupts")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/prep.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 0a9162fba97..7481bbf99d4 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -230,6 +230,15 @@ static void raven_change_gpio(void *opaque, int n, int level)
     s->contiguous_map = level;
 }
 
+static void raven_pcihost_reset_enter(Object *obj, ResetType type)
+{
+    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
+
+    if (!s->is_legacy_prep) {
+        device_cold_reset(DEVICE(&s->or_irq));
+    }
+}
+
 static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 {
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
@@ -419,11 +428,13 @@ static Property raven_pcihost_properties[] = {
 static void raven_pcihost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->realize = raven_pcihost_realizefn;
     device_class_set_props(dc, raven_pcihost_properties);
     dc->fw_name = "pci";
+    rc->phases.enter = raven_pcihost_reset_enter;
 }
 
 static const TypeInfo raven_pcihost_info = {
-- 
2.26.3

Re: [PATCH v2 2/3] hw/pci-host/raven: Manually reset the OR_IRQ device

[Peter Maydell]

On Sun, 2 May 2021 at 21:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The OR_IRQ device is bus-less, thus isn't reset automatically.
> Add the raven_pcihost_reset() handler to manually reset the OR IRQ.
>
> Fixes: f40b83a4e31 ("40p: use OR gate to wire up raven PCI interrupts")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/pci-host/prep.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 0a9162fba97..7481bbf99d4 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -230,6 +230,15 @@ static void raven_change_gpio(void *opaque, int n, int level)
>      s->contiguous_map = level;
>  }
>
> +static void raven_pcihost_reset_enter(Object *obj, ResetType type)
> +{
> +    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
> +
> +    if (!s->is_legacy_prep) {
> +        device_cold_reset(DEVICE(&s->or_irq));
> +    }
> +}
> +
>  static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>  {
>      SysBusDevice *dev = SYS_BUS_DEVICE(d);
> @@ -419,11 +428,13 @@ static Property raven_pcihost_properties[] = {
>  static void raven_pcihost_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
>
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->realize = raven_pcihost_realizefn;
>      device_class_set_props(dc, raven_pcihost_properties);
>      dc->fw_name = "pci";
> +    rc->phases.enter = raven_pcihost_reset_enter;
>  }

Why does this device have an OR gate rather than having its
map_irq function say "all PCI IRQs go to interrupt 0" ?
(The PCI core code provides you the "OR" functionality for
free, because it has to do that anyway for when multiple PCI
cards share a PCI IRQ -- see pci_change_irq_level() and
pci_bus_change_irq_level().

Supplementary question: why does the legacy_prep setup create 4
outbound sysbus IRQs when the map_irq function can only ever
return 0 or 1 ?

thanks
-- PMM

Re: [PATCH v2 2/3] hw/pci-host/raven: Manually reset the OR_IRQ device

[Philippe Mathieu-Daudé]

On 5/2/21 10:45 PM, Peter Maydell wrote:
> On Sun, 2 May 2021 at 21:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The OR_IRQ device is bus-less, thus isn't reset automatically.
>> Add the raven_pcihost_reset() handler to manually reset the OR IRQ.
>>
>> Fixes: f40b83a4e31 ("40p: use OR gate to wire up raven PCI interrupts")
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/pci-host/prep.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>> index 0a9162fba97..7481bbf99d4 100644
>> --- a/hw/pci-host/prep.c
>> +++ b/hw/pci-host/prep.c
>> @@ -230,6 +230,15 @@ static void raven_change_gpio(void *opaque, int n, int level)
>>      s->contiguous_map = level;
>>  }
>>
>> +static void raven_pcihost_reset_enter(Object *obj, ResetType type)
>> +{
>> +    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>> +
>> +    if (!s->is_legacy_prep) {
>> +        device_cold_reset(DEVICE(&s->or_irq));
>> +    }
>> +}
>> +
>>  static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>  {
>>      SysBusDevice *dev = SYS_BUS_DEVICE(d);
>> @@ -419,11 +428,13 @@ static Property raven_pcihost_properties[] = {
>>  static void raven_pcihost_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
>>
>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>      dc->realize = raven_pcihost_realizefn;
>>      device_class_set_props(dc, raven_pcihost_properties);
>>      dc->fw_name = "pci";
>> +    rc->phases.enter = raven_pcihost_reset_enter;
>>  }
> 
> Why does this device have an OR gate rather than having its
> map_irq function say "all PCI IRQs go to interrupt 0" ?
> (The PCI core code provides you the "OR" functionality for
> free, because it has to do that anyway for when multiple PCI
> cards share a PCI IRQ -- see pci_change_irq_level() and
> pci_bus_change_irq_level().
> 
> Supplementary question: why does the legacy_prep setup create 4
> outbound sysbus IRQs when the map_irq function can only ever
> return 0 or 1 ?

I'll let the maintainers have a look (I'm just trying to pay the
technical debts so we can remove legacy/deprecated API and move
forward).

Thanks for the careful analysis,

Phil.

Re: [PATCH v2 2/3] hw/pci-host/raven: Manually reset the OR_IRQ device

[David Gibson]

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

On Sun, May 02, 2021 at 10:31:20PM +0200, Philippe Mathieu-Daudé wrote:
> The OR_IRQ device is bus-less, thus isn't reset automatically.
> Add the raven_pcihost_reset() handler to manually reset the OR IRQ.
> 
> Fixes: f40b83a4e31 ("40p: use OR gate to wire up raven PCI interrupts")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/pci-host/prep.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 0a9162fba97..7481bbf99d4 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -230,6 +230,15 @@ static void raven_change_gpio(void *opaque, int n, int level)
>      s->contiguous_map = level;
>  }
>  
> +static void raven_pcihost_reset_enter(Object *obj, ResetType type)
> +{
> +    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
> +
> +    if (!s->is_legacy_prep) {
> +        device_cold_reset(DEVICE(&s->or_irq));
> +    }
> +}
> +
>  static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>  {
>      SysBusDevice *dev = SYS_BUS_DEVICE(d);
> @@ -419,11 +428,13 @@ static Property raven_pcihost_properties[] = {
>  static void raven_pcihost_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
>  
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->realize = raven_pcihost_realizefn;
>      device_class_set_props(dc, raven_pcihost_properties);
>      dc->fw_name = "pci";
> +    rc->phases.enter = raven_pcihost_reset_enter;
>  }
>  
>  static const TypeInfo raven_pcihost_info = {

-- 
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 v2 3/3] hw/arm/armsse: Manually reset the OR_IRQ devices

[Philippe Mathieu-Daudé]

The OR_IRQ device is bus-less, thus isn't reset automatically.
Manually reset the OR IRQs in the armsse_reset() handler.

Fixes: bb75e16d5e6 ("hw/arm/iotkit: Wire up MPC interrupt lines")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/armsse.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 2e5d0679e7b..a5f8e89950e 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -1668,6 +1668,10 @@ static void armsse_reset(DeviceState *dev)
     ARMSSE *s = ARM_SSE(dev);
 
     s->nsccfg = 0;
+
+    device_cold_reset(DEVICE(&s->mpc_irq_orgate));
+    device_cold_reset(DEVICE(&s->ppc_irq_orgate));
+    device_cold_reset(DEVICE(&s->sec_resp_splitter));
 }
 
 static void armsse_class_init(ObjectClass *klass, void *data)
-- 
2.26.3

Re: [PATCH v2 3/3] hw/arm/armsse: Manually reset the OR_IRQ devices

[Peter Maydell]

On Sun, 2 May 2021 at 21:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The OR_IRQ device is bus-less, thus isn't reset automatically.
> Manually reset the OR IRQs in the armsse_reset() handler.
>
> Fixes: bb75e16d5e6 ("hw/arm/iotkit: Wire up MPC interrupt lines")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/armsse.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
> index 2e5d0679e7b..a5f8e89950e 100644
> --- a/hw/arm/armsse.c
> +++ b/hw/arm/armsse.c
> @@ -1668,6 +1668,10 @@ static void armsse_reset(DeviceState *dev)
>      ARMSSE *s = ARM_SSE(dev);
>
>      s->nsccfg = 0;
> +
> +    device_cold_reset(DEVICE(&s->mpc_irq_orgate));
> +    device_cold_reset(DEVICE(&s->ppc_irq_orgate));
> +    device_cold_reset(DEVICE(&s->sec_resp_splitter));
>  }

I guess we should put in this temporary fix pending figuring out
the better more systematic one.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM