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

[PATCH 0/5] 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].

I'm still confused by the TYPE_APIC (and its KVM version), see [3].

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg801374.html
[2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg800917.html
[3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg801379.html

Philippe Mathieu-Daudé (5):
  hw/ppc/spapr_iommu: Register machine reset handler
  hw/pcmcia/microdrive: Register machine reset handler
  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   | 10 ++++++++++
 hw/pcmcia/pcmcia.c   | 25 +++++++++++++++++++++++++
 hw/ppc/spapr_iommu.c | 10 ++++++++++
 5 files changed, 63 insertions(+)

-- 
2.26.3

[PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler

[Philippe Mathieu-Daudé]

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

It doesn't seem to be an issue because it is that way since the
device QDev'ifycation 8 years ago, in commit a83000f5e3f
("spapr-tce: make sPAPRTCETable a proper device").
Still, correct to have a proper API usage.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/spapr_iommu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 24537ffcbd3..f7dad1dc0fe 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -24,6 +24,7 @@
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
 #include "migration/vmstate.h"
+#include "sysemu/reset.h"
 #include "sysemu/dma.h"
 #include "exec/address-spaces.h"
 #include "trace.h"
@@ -302,6 +303,11 @@ static const VMStateDescription vmstate_spapr_tce_table = {
     }
 };
 
+static void spapr_tce_reset_handler(void *dev)
+{
+    device_legacy_reset(DEVICE(dev));
+}
+
 static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
 {
     SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev);
@@ -324,6 +330,8 @@ static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
 
     vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,
                      tcet);
+
+    qemu_register_reset(spapr_tce_reset_handler, dev);
 }
 
 void spapr_tce_set_need_vfio(SpaprTceTable *tcet, bool need_vfio)
@@ -425,6 +433,8 @@ static void spapr_tce_table_unrealize(DeviceState *dev)
 {
     SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev);
 
+    qemu_unregister_reset(spapr_tce_reset_handler, dev);
+
     vmstate_unregister(VMSTATE_IF(tcet), &vmstate_spapr_tce_table, tcet);
 
     QLIST_REMOVE(tcet, list);
-- 
2.26.3

[PATCH 2/5] hw/pcmcia/microdrive: Register machine reset handler

[Philippe Mathieu-Daudé]

The abstract PCMCIA_CARD is a bus-less TYPE_DEVICE, so devices
implementing it are not reset automatically.
Register a reset handler so children get reset on machine reset.

Note, the DSCM-1XXXX device (TYPE_DSCM1XXXX) which inherits
TYPE_MICRODRIVE and PCMCIA_CARD reset itself when a disk is
attached or detached, but was not resetting itself on machine
reset.

It doesn't seem to be an issue because it is that way since the
device QDev'ifycation 8 years ago, in commit d1f2c96a81a
("pcmcia: QOM'ify PCMCIACardState and MicroDriveState").
Still, correct to have a proper API usage.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pcmcia/pcmcia.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/pcmcia/pcmcia.c b/hw/pcmcia/pcmcia.c
index 03d13e7d670..73656257227 100644
--- a/hw/pcmcia/pcmcia.c
+++ b/hw/pcmcia/pcmcia.c
@@ -6,14 +6,39 @@
 
 #include "qemu/osdep.h"
 #include "qemu/module.h"
+#include "sysemu/reset.h"
 #include "hw/pcmcia.h"
 
+static void pcmcia_card_reset_handler(void *dev)
+{
+    device_legacy_reset(DEVICE(dev));
+}
+
+static void pcmcia_card_realize(DeviceState *dev, Error **errp)
+{
+    qemu_register_reset(pcmcia_card_reset_handler, dev);
+}
+
+static void pcmcia_card_unrealize(DeviceState *dev)
+{
+    qemu_unregister_reset(pcmcia_card_reset_handler, dev);
+}
+
+static void pcmcia_card_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = pcmcia_card_realize;
+    dc->unrealize = pcmcia_card_unrealize;
+}
+
 static const TypeInfo pcmcia_card_type_info = {
     .name = TYPE_PCMCIA_CARD,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(PCMCIACardState),
     .abstract = true,
     .class_size = sizeof(PCMCIACardClass),
+    .class_init = pcmcia_card_class_init,
 };
 
 static void pcmcia_register_types(void)
-- 
2.26.3

[PATCH 3/5] 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..d3fb5107bfe 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_legacy_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

[PATCH 4/5] 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 0a9162fba97..275379e4c78 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(DeviceState *dev)
+{
+    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
+
+    if (!s->is_legacy_prep) {
+        device_legacy_reset(DEVICE(&s->or_irq));
+    }
+}
+
 static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 {
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
@@ -422,6 +431,7 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data)
 
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->realize = raven_pcihost_realizefn;
+    dc->reset = raven_pcihost_reset;
     device_class_set_props(dc, raven_pcihost_properties);
     dc->fw_name = "pci";
 }
-- 
2.26.3

[PATCH 5/5] 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..7fd3fb77620 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_legacy_reset(DEVICE(&s->mpc_irq_orgate));
+    device_legacy_reset(DEVICE(&s->ppc_irq_orgate));
+    device_legacy_reset(DEVICE(&s->sec_resp_splitter));
 }
 
 static void armsse_class_init(ObjectClass *klass, void *data)
-- 
2.26.3

Re: [PATCH 2/5] hw/pcmcia/microdrive: Register machine reset handler

[Peter Maydell]

On Sat, 24 Apr 2021 at 17:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The abstract PCMCIA_CARD is a bus-less TYPE_DEVICE, so devices
> implementing it are not reset automatically.
> Register a reset handler so children get reset on machine reset.
>
> Note, the DSCM-1XXXX device (TYPE_DSCM1XXXX) which inherits
> TYPE_MICRODRIVE and PCMCIA_CARD reset itself when a disk is
> attached or detached, but was not resetting itself on machine
> reset.
>
> It doesn't seem to be an issue because it is that way since the
> device QDev'ifycation 8 years ago, in commit d1f2c96a81a
> ("pcmcia: QOM'ify PCMCIACardState and MicroDriveState").
> Still, correct to have a proper API usage.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/pcmcia/pcmcia.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/hw/pcmcia/pcmcia.c b/hw/pcmcia/pcmcia.c
> index 03d13e7d670..73656257227 100644
> --- a/hw/pcmcia/pcmcia.c
> +++ b/hw/pcmcia/pcmcia.c
> @@ -6,14 +6,39 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
> +#include "sysemu/reset.h"
>  #include "hw/pcmcia.h"
>
> +static void pcmcia_card_reset_handler(void *dev)
> +{
> +    device_legacy_reset(DEVICE(dev));
> +}
> +
> +static void pcmcia_card_realize(DeviceState *dev, Error **errp)
> +{
> +    qemu_register_reset(pcmcia_card_reset_handler, dev);
> +}
> +
> +static void pcmcia_card_unrealize(DeviceState *dev)
> +{
> +    qemu_unregister_reset(pcmcia_card_reset_handler, dev);
> +}

Why isn't a pcmcia card something that plugs into a bus ?

-- PMM

Re: [PATCH 2/5] hw/pcmcia/microdrive: Register machine reset handler

[Philippe Mathieu-Daudé]

On 4/25/21 8:36 PM, Peter Maydell wrote:
> On Sat, 24 Apr 2021 at 17:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The abstract PCMCIA_CARD is a bus-less TYPE_DEVICE, so devices
>> implementing it are not reset automatically.
>> Register a reset handler so children get reset on machine reset.
>>
>> Note, the DSCM-1XXXX device (TYPE_DSCM1XXXX) which inherits
>> TYPE_MICRODRIVE and PCMCIA_CARD reset itself when a disk is
>> attached or detached, but was not resetting itself on machine
>> reset.
>>
>> It doesn't seem to be an issue because it is that way since the
>> device QDev'ifycation 8 years ago, in commit d1f2c96a81a
>> ("pcmcia: QOM'ify PCMCIACardState and MicroDriveState").
>> Still, correct to have a proper API usage.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/pcmcia/pcmcia.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/hw/pcmcia/pcmcia.c b/hw/pcmcia/pcmcia.c
>> index 03d13e7d670..73656257227 100644
>> --- a/hw/pcmcia/pcmcia.c
>> +++ b/hw/pcmcia/pcmcia.c
>> @@ -6,14 +6,39 @@
>>
>>  #include "qemu/osdep.h"
>>  #include "qemu/module.h"
>> +#include "sysemu/reset.h"
>>  #include "hw/pcmcia.h"
>>
>> +static void pcmcia_card_reset_handler(void *dev)
>> +{
>> +    device_legacy_reset(DEVICE(dev));
>> +}
>> +
>> +static void pcmcia_card_realize(DeviceState *dev, Error **errp)
>> +{
>> +    qemu_register_reset(pcmcia_card_reset_handler, dev);
>> +}
>> +
>> +static void pcmcia_card_unrealize(DeviceState *dev)
>> +{
>> +    qemu_unregister_reset(pcmcia_card_reset_handler, dev);
>> +}
> 
> Why isn't a pcmcia card something that plugs into a bus ?

No clue, looks like a very old device with unfinished qdev-ification?

See pxa2xx_pcmcia_attach():

/* Insert a new card into a slot */
int pxa2xx_pcmcia_attach(void *opaque, PCMCIACardState *card)
{
    PXA2xxPCMCIAState *s = (PXA2xxPCMCIAState *) opaque;
    PCMCIACardClass *pcc;

    ...
    s->card = card;
    pcc = PCMCIA_CARD_GET_CLASS(s->card);
    ...
    s->card->slot = &s->slot;
    pcc->attach(s->card);
    ...
}

Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler

[David Gibson]

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

On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote:
> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset
> automatically.  Register a reset handler to get reset with the
> machine.
> 
> It doesn't seem to be an issue because it is that way since the
> device QDev'ifycation 8 years ago, in commit a83000f5e3f
> ("spapr-tce: make sPAPRTCETable a proper device").
> Still, correct to have a proper API usage.

So, the reason this works now is that we explicitly call
device_reset() on the TCE table from the TCE tables "owner", either a
PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()).

I think we want either that, or the register_reset(), not both.

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ppc/spapr_iommu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 24537ffcbd3..f7dad1dc0fe 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -24,6 +24,7 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_ppc.h"
>  #include "migration/vmstate.h"
> +#include "sysemu/reset.h"
>  #include "sysemu/dma.h"
>  #include "exec/address-spaces.h"
>  #include "trace.h"
> @@ -302,6 +303,11 @@ static const VMStateDescription vmstate_spapr_tce_table = {
>      }
>  };
>  
> +static void spapr_tce_reset_handler(void *dev)
> +{
> +    device_legacy_reset(DEVICE(dev));
> +}
> +
>  static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
>  {
>      SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev);
> @@ -324,6 +330,8 @@ static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
>  
>      vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,
>                       tcet);
> +
> +    qemu_register_reset(spapr_tce_reset_handler, dev);
>  }
>  
>  void spapr_tce_set_need_vfio(SpaprTceTable *tcet, bool need_vfio)
> @@ -425,6 +433,8 @@ static void spapr_tce_table_unrealize(DeviceState *dev)
>  {
>      SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev);
>  
> +    qemu_unregister_reset(spapr_tce_reset_handler, dev);
> +
>      vmstate_unregister(VMSTATE_IF(tcet), &vmstate_spapr_tce_table, tcet);
>  
>      QLIST_REMOVE(tcet, list);

-- 
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 4/5] hw/pci-host/raven: Manually reset the OR_IRQ device

[David Gibson]

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

On Sat, Apr 24, 2021 at 06:22:28PM +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 | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 0a9162fba97..275379e4c78 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(DeviceState *dev)
> +{
> +    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
> +
> +    if (!s->is_legacy_prep) {
> +        device_legacy_reset(DEVICE(&s->or_irq));
> +    }
> +}
> +
>  static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>  {
>      SysBusDevice *dev = SYS_BUS_DEVICE(d);
> @@ -422,6 +431,7 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data)
>  
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->realize = raven_pcihost_realizefn;
> +    dc->reset = raven_pcihost_reset;
>      device_class_set_props(dc, raven_pcihost_properties);
>      dc->fw_name = "pci";
>  }

-- 
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 1/5] hw/ppc/spapr_iommu: Register machine reset handler

[Philippe Mathieu-Daudé]

On 4/27/21 3:45 AM, David Gibson wrote:
> On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote:
>> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset
>> automatically.  Register a reset handler to get reset with the
>> machine.
>>
>> It doesn't seem to be an issue because it is that way since the
>> device QDev'ifycation 8 years ago, in commit a83000f5e3f
>> ("spapr-tce: make sPAPRTCETable a proper device").
>> Still, correct to have a proper API usage.
> 
> So, the reason this works now is that we explicitly call
> device_reset() on the TCE table from the TCE tables "owner", either a
> PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()).
> 
> I think we want either that, or the register_reset(), not both.

rtas_quiesce() seems to call a DeviceClass::reset() on the
children of TYPE_SPAPR_VIO_BUS:

Abstract TYPE_VIO_SPAPR_DEVICE has the TYPE_SPAPR_VIO_BUS bus_type,
and registers the spapr_vio_busdev_reset() handler, which calls
spapr_vio_quiesce_one()...

So either we already have 2 resets, or the bus is never reset?

The bus is created in spapr_machine_init():

    /* Set up VIO bus */
    spapr->vio_bus = spapr_vio_bus_init();

TYPE_SPAPR_MACHINE class registers spapr_machine_reset(), which
manually calls qemu_devices_reset() and spapr_drc_reset_all(),
but I can't understand if a callee resets vio_bus...

Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler

[Greg Kurz]

On Tue, 27 Apr 2021 11:20:07 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 4/27/21 3:45 AM, David Gibson wrote:
> > On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote:
> >> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset
> >> automatically.  Register a reset handler to get reset with the
> >> machine.
> >>
> >> It doesn't seem to be an issue because it is that way since the
> >> device QDev'ifycation 8 years ago, in commit a83000f5e3f
> >> ("spapr-tce: make sPAPRTCETable a proper device").
> >> Still, correct to have a proper API usage.
> > 
> > So, the reason this works now is that we explicitly call
> > device_reset() on the TCE table from the TCE tables "owner", either a
> > PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()).
> > 
> > I think we want either that, or the register_reset(), not both.
> 
> rtas_quiesce() seems to call a DeviceClass::reset() on the
> children of TYPE_SPAPR_VIO_BUS:
> 
> Abstract TYPE_VIO_SPAPR_DEVICE has the TYPE_SPAPR_VIO_BUS bus_type,
> and registers the spapr_vio_busdev_reset() handler, which calls
> spapr_vio_quiesce_one()...
> 
> So either we already have 2 resets, or the bus is never reset?
> 

rtas_quiesce() is called when the guests definitively transition
from the SLOF FW to the OS. It isn't a true reset path actually,
even if it needs to reset a few devices.

On the other hand, your patch would _really_ cause the TCE table
device to be reset twice at machine reset AFAICT.

> The bus is created in spapr_machine_init():
> 
>     /* Set up VIO bus */
>     spapr->vio_bus = spapr_vio_bus_init();
> 
> TYPE_SPAPR_MACHINE class registers spapr_machine_reset(), which
> manually calls qemu_devices_reset() and spapr_drc_reset_all(),
> but I can't understand if a callee resets vio_bus...

The vio_bus *is* reset:

#0  0x0000000100629a98 in spapr_vio_busdev_reset (qdev=0x10165c400) at /home/greg/Work/qemu/qemu-virtiofs/include/hw/ppc/spapr_vio.h:31
#1  0x00000001009fd32c in device_transitional_reset (obj=0x10165c400) at /home/greg/Work/qemu/qemu-virtiofs/include/hw/qdev-core.h:17
#2  0x0000000100a00e24 in resettable_phase_hold (obj=0x10165c400, opaque=<optimized out>, type=<optimized out>) at ../../hw/core/resettable.c:182
#3  0x00000001009f9108 in bus_reset_child_foreach (obj=<optimized out>, cb=0x100a00cc0 <resettable_phase_hold>, opaque=0x0, type=<optimized out>) at ../../hw/core/bus.c:97
#4  0x0000000100a00db8 in resettable_child_foreach (rc=0x1014f5400, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 <resettable_phase_hold>, obj=0x10156e600) at ../../hw/core/resettable.c:96
#5  0x0000000100a00db8 in resettable_phase_hold (obj=0x10156e600, opaque=<optimized out>, type=<optimized out>) at ../../hw/core/resettable.c:173
#6  0x00000001009fcaa8 in device_reset_child_foreach (obj=<optimized out>, cb=0x100a00cc0 <resettable_phase_hold>, opaque=0x0, type=<optimized out>) at ../../hw/core/qdev.c:366
#7  0x0000000100a00db8 in resettable_child_foreach (rc=0x1013eef90, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 <resettable_phase_hold>, obj=0x10164a0e0) at ../../hw/core/resettable.c:96
#8  0x0000000100a00db8 in resettable_phase_hold (obj=0x10164a0e0, opaque=<optimized out>, type=<optimized out>) at ../../hw/core/resettable.c:173
#9  0x00000001009f9108 in bus_reset_child_foreach (obj=<optimized out>, cb=0x100a00cc0 <resettable_phase_hold>, opaque=0x0, type=<optimized out>) at ../../hw/core/bus.c:97
#10 0x0000000100a00db8 in resettable_child_foreach (rc=0x1012b1a00, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 <resettable_phase_hold>, obj=0x10154d4b0) at ../../hw/core/resettable.c:96
#11 0x0000000100a00db8 in resettable_phase_hold (obj=obj@entry=0x10154d4b0, opaque=opaque@entry=0x0, type=type@entry=RESET_TYPE_COLD) at ../../hw/core/resettable.c:173
#12 0x0000000100a01794 in resettable_assert_reset (obj=0x10154d4b0, type=<optimized out>) at ../../hw/core/resettable.c:60
#13 0x0000000100a01c60 in resettable_reset (obj=0x10154d4b0, type=<optimized out>) at ../../hw/core/resettable.c:45
#14 0x0000000100a020ec in resettable_cold_reset_fn (opaque=<optimized out>) at ../../hw/core/resettable.c:269
#15 0x0000000100a00718 in qemu_devices_reset () at ../../hw/core/reset.c:69
#16 0x0000000100624024 in spapr_machine_reset (machine=0x101545480) at ../../hw/ppc/spapr.c:1587
#17 0x00000001007b8128 in qemu_system_reset (reason=<optimized out>) at ../../softmmu/runstate.c:442
#18 0x00000001007b8fa8 in main_loop_should_exit () at ../../softmmu/runstate.c:687
#19 0x00000001007b8fa8 in qemu_main_loop () at ../../softmmu/runstate.c:721
#20 0x00000001002f5150 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at ../../softmmu/main.c:50

And it seems rtas_quiesce() could just do bus_cold_reset(&bus->bus)
rather than open-coding the walk of vio_bus children.

Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler

[David Gibson]

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

On Tue, Apr 27, 2021 at 11:20:07AM +0200, Philippe Mathieu-Daudé wrote:
> On 4/27/21 3:45 AM, David Gibson wrote:
> > On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote:
> >> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset
> >> automatically.  Register a reset handler to get reset with the
> >> machine.
> >>
> >> It doesn't seem to be an issue because it is that way since the
> >> device QDev'ifycation 8 years ago, in commit a83000f5e3f
> >> ("spapr-tce: make sPAPRTCETable a proper device").
> >> Still, correct to have a proper API usage.
> > 
> > So, the reason this works now is that we explicitly call
> > device_reset() on the TCE table from the TCE tables "owner", either a
> > PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()).
> > 
> > I think we want either that, or the register_reset(), not both.
> 
> rtas_quiesce() seems to call a DeviceClass::reset() on the
> children of TYPE_SPAPR_VIO_BUS:
> 
> Abstract TYPE_VIO_SPAPR_DEVICE has the TYPE_SPAPR_VIO_BUS bus_type,
> and registers the spapr_vio_busdev_reset() handler, which calls
> spapr_vio_quiesce_one()...
> 
> So either we already have 2 resets, or the bus is never reset?

There are 2 resets, and this is intentional.  We reset once at machine
reset time, via the bus.  Once a booting OS is done with the firmware
it calls "quiesce" to put all the devices back into a safe state.  The
easiest way to do that is just to invoke their reset callbacks, so
that's what we do.

> The bus is created in spapr_machine_init():
> 
>     /* Set up VIO bus */
>     spapr->vio_bus = spapr_vio_bus_init();
> 
> TYPE_SPAPR_MACHINE class registers spapr_machine_reset(), which
> manually calls qemu_devices_reset() and spapr_drc_reset_all(),
> but I can't understand if a callee resets vio_bus...
> 

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