[Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Philippe Mathieu-Daudé]

Hi Christian,

I'm having hard time to understand why the S390_IPL object calls
qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
being QOM'ified (it has a reset method).

It doesn't seem to have a qdev children added explicitly to it.
I see it is used as a singleton, what else am I missing?

Thanks,

Phil.

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[David Hildenbrand]

On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:
> Hi Christian,
> 
> I'm having hard time to understand why the S390_IPL object calls
> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
> being QOM'ified (it has a reset method).
> 
> It doesn't seem to have a qdev children added explicitly to it.
> I see it is used as a singleton, what else am I missing?
> 
> Thanks,
> 
> Phil.
> 

Looks like I added it back then (~4 years ago) when converting it into a
TYPE_DEVICE.

I could imagine that - back then - this was needed because only
TYPE_SYS_BUS_DEVICE would recursively get reset.

Did you try removing it, to see if anything breaks?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Philippe Mathieu-Daudé]

On 5/24/19 8:04 PM, David Hildenbrand wrote:
> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:
>> Hi Christian,
>>
>> I'm having hard time to understand why the S390_IPL object calls
>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>> being QOM'ified (it has a reset method).
>>
>> It doesn't seem to have a qdev children added explicitly to it.
>> I see it is used as a singleton, what else am I missing?
>>
>> Thanks,
>>
>> Phil.
>>
> 
> Looks like I added it back then (~4 years ago) when converting it into a
> TYPE_DEVICE.
> 
> I could imagine that - back then - this was needed because only
> TYPE_SYS_BUS_DEVICE would recursively get reset.

Thanks for the quick response :)

> Did you try removing it, to see if anything breaks?

From build POV it is OK, but I have now idea of the effects with KVM.

I don't know how to test on s390x systems, but luckily Patchew/s390x is
back up so I'll try my luck with a RFC patch, but I'm not sure the
default tests cover this specific device uses.

Regards,

Phil.

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Christian Borntraeger]

On 24.05.19 20:04, David Hildenbrand wrote:
> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:
>> Hi Christian,
>>
>> I'm having hard time to understand why the S390_IPL object calls
>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>> being QOM'ified (it has a reset method).
>>
>> It doesn't seem to have a qdev children added explicitly to it.
>> I see it is used as a singleton, what else am I missing?
>>
>> Thanks,
>>
>> Phil.
>>
> 
> Looks like I added it back then (~4 years ago) when converting it into a
> TYPE_DEVICE.
> 
> I could imagine that - back then - this was needed because only
> TYPE_SYS_BUS_DEVICE would recursively get reset.

Yes, back then singleton devices were not recursively resetted. Has that changed?
> 
> Did you try removing it, to see if anything breaks?
> 

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[David Hildenbrand]

On 24.05.19 20:28, Christian Borntraeger wrote:
> 
> 
> On 24.05.19 20:04, David Hildenbrand wrote:
>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:
>>> Hi Christian,
>>>
>>> I'm having hard time to understand why the S390_IPL object calls
>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>>> being QOM'ified (it has a reset method).
>>>
>>> It doesn't seem to have a qdev children added explicitly to it.
>>> I see it is used as a singleton, what else am I missing?
>>>
>>> Thanks,
>>>
>>> Phil.
>>>
>>
>> Looks like I added it back then (~4 years ago) when converting it into a
>> TYPE_DEVICE.
>>
>> I could imagine that - back then - this was needed because only
>> TYPE_SYS_BUS_DEVICE would recursively get reset.
> 
> Yes, back then singleton devices were not recursively resetted. Has that changed?

Hacking that call out, I don't see it getting called anymore. So it is
still required. The question is if it can be reworked.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[David Hildenbrand]

On 24.05.19 20:36, David Hildenbrand wrote:
> On 24.05.19 20:28, Christian Borntraeger wrote:
>>
>>
>> On 24.05.19 20:04, David Hildenbrand wrote:
>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:
>>>> Hi Christian,
>>>>
>>>> I'm having hard time to understand why the S390_IPL object calls
>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>>>> being QOM'ified (it has a reset method).
>>>>
>>>> It doesn't seem to have a qdev children added explicitly to it.
>>>> I see it is used as a singleton, what else am I missing?
>>>>
>>>> Thanks,
>>>>
>>>> Phil.
>>>>
>>>
>>> Looks like I added it back then (~4 years ago) when converting it into a
>>> TYPE_DEVICE.
>>>
>>> I could imagine that - back then - this was needed because only
>>> TYPE_SYS_BUS_DEVICE would recursively get reset.
>>
>> Yes, back then singleton devices were not recursively resetted. Has that changed?
> 
> Hacking that call out, I don't see it getting called anymore. So it is
> still required. The question is if it can be reworked.
> 

Yes, as it is not a sysbus device, it won't get reset.
The owner (machine) has to take care of this. The following works:


diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index b93750c14e..91a31c2cd0 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
      */
     ipl->compat_start_addr = ipl->start_addr;
     ipl->compat_bios_start_addr = ipl->bios_start_addr;
-    qemu_register_reset(qdev_reset_all_fn, dev);
 error:
     error_propagate(errp, err);
 }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index bbc6e8fa0b..658ab529a1 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
     s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
 }
 
+static void s390_ipl_reset(void)
+{
+    qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL)));
+}
+
 static void s390_machine_reset(void)
 {
     enum s390_reset reset_type;
@@ -353,6 +358,7 @@ static void s390_machine_reset(void)
     case S390_RESET_EXTERNAL:
     case S390_RESET_REIPL:
         qemu_devices_reset();
+        s390_ipl_reset();
         s390_crypto_reset();
 
         /* configure and start the ipl CPU only */



-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Christian Borntraeger]

On 24.05.19 21:00, David Hildenbrand wrote:
> On 24.05.19 20:36, David Hildenbrand wrote:
>> On 24.05.19 20:28, Christian Borntraeger wrote:
>>>
>>>
>>> On 24.05.19 20:04, David Hildenbrand wrote:
>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:
>>>>> Hi Christian,
>>>>>
>>>>> I'm having hard time to understand why the S390_IPL object calls
>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>>>>> being QOM'ified (it has a reset method).
>>>>>
>>>>> It doesn't seem to have a qdev children added explicitly to it.
>>>>> I see it is used as a singleton, what else am I missing?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Phil.
>>>>>
>>>>
>>>> Looks like I added it back then (~4 years ago) when converting it into a
>>>> TYPE_DEVICE.
>>>>
>>>> I could imagine that - back then - this was needed because only
>>>> TYPE_SYS_BUS_DEVICE would recursively get reset.
>>>
>>> Yes, back then singleton devices were not recursively resetted. Has that changed?
>>
>> Hacking that call out, I don't see it getting called anymore. So it is
>> still required. The question is if it can be reworked.
>>
> 
> Yes, as it is not a sysbus device, it won't get reset.
> The owner (machine) has to take care of this. The following works:
> 
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index b93750c14e..91a31c2cd0 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>       */
>      ipl->compat_start_addr = ipl->start_addr;
>      ipl->compat_bios_start_addr = ipl->bios_start_addr;
> -    qemu_register_reset(qdev_reset_all_fn, dev);
>  error:
>      error_propagate(errp, err);
>  }
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index bbc6e8fa0b..658ab529a1 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>  }
>  
> +static void s390_ipl_reset(void)
> +{
> +    qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL)));
> +}
> +
>  static void s390_machine_reset(void)
>  {
>      enum s390_reset reset_type;
> @@ -353,6 +358,7 @@ static void s390_machine_reset(void)
>      case S390_RESET_EXTERNAL:
>      case S390_RESET_REIPL:
>          qemu_devices_reset();
> +        s390_ipl_reset();
>          s390_crypto_reset();
>  
>          /* configure and start the ipl CPU only */
> 

While this patch is certainly ok, I find it disturbing that qdev devices are being resetted,
but qom devices not.

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[David Hildenbrand]

On 24.05.19 21:45, Christian Borntraeger wrote:
> 
> 
> On 24.05.19 21:00, David Hildenbrand wrote:
>> On 24.05.19 20:36, David Hildenbrand wrote:
>>> On 24.05.19 20:28, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 24.05.19 20:04, David Hildenbrand wrote:
>>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:
>>>>>> Hi Christian,
>>>>>>
>>>>>> I'm having hard time to understand why the S390_IPL object calls
>>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>>>>>> being QOM'ified (it has a reset method).
>>>>>>
>>>>>> It doesn't seem to have a qdev children added explicitly to it.
>>>>>> I see it is used as a singleton, what else am I missing?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Phil.
>>>>>>
>>>>>
>>>>> Looks like I added it back then (~4 years ago) when converting it into a
>>>>> TYPE_DEVICE.
>>>>>
>>>>> I could imagine that - back then - this was needed because only
>>>>> TYPE_SYS_BUS_DEVICE would recursively get reset.
>>>>
>>>> Yes, back then singleton devices were not recursively resetted. Has that changed?
>>>
>>> Hacking that call out, I don't see it getting called anymore. So it is
>>> still required. The question is if it can be reworked.
>>>
>>
>> Yes, as it is not a sysbus device, it won't get reset.
>> The owner (machine) has to take care of this. The following works:
>>
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index b93750c14e..91a31c2cd0 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>       */
>>      ipl->compat_start_addr = ipl->start_addr;
>>      ipl->compat_bios_start_addr = ipl->bios_start_addr;
>> -    qemu_register_reset(qdev_reset_all_fn, dev);
>>  error:
>>      error_propagate(errp, err);
>>  }
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index bbc6e8fa0b..658ab529a1 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
>>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>  }
>>  
>> +static void s390_ipl_reset(void)
>> +{
>> +    qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL)));
>> +}
>> +
>>  static void s390_machine_reset(void)
>>  {
>>      enum s390_reset reset_type;
>> @@ -353,6 +358,7 @@ static void s390_machine_reset(void)
>>      case S390_RESET_EXTERNAL:
>>      case S390_RESET_REIPL:
>>          qemu_devices_reset();
>> +        s390_ipl_reset();
>>          s390_crypto_reset();
>>  
>>          /* configure and start the ipl CPU only */
>>
> 
> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted,
> but qom devices not.

I guess the disturbing thing is "... after all these years" :)


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Peter Maydell]

On Fri, 24 May 2019 at 20:47, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted,
> but qom devices not.

It's not a qdev-vs-QOM thing. Anything which is a DeviceState
has a reset method, but only devices which are somewhere
rooted in the bus-tree that starts with the "main system
bus" (aka sysbus) get reset by the vl.c-registered "reset
everything on the system bus". Devices which are SysBusDevice
get auto-parented onto the sysbus, and so get reset. Devices
like PCI devices or SCSI devices get put onto the PCI
bus or the SCSI bus, and those buses are in turn children
of some host-controller device which is on the sysbus, so
they all get reset. The things that don't get reset are
"orphan" devices which are neither (a) of a type that gets
automatically parented onto a bus like SysBusDevice nor
(b) put specifically onto some other bus.

CPU objects are the other common thing that doesn't get
reset 'automatically'.

Suggestions for how to restructure reset so this doesn't
happen are welcome... "reset follows the bus hierarchy"
works well in some places but is a bit weird in others
(for SoC containers and the like "follow the QOM
hierarchy" would make more sense, but I have no idea
how to usefully transition to a model where you could
say "for these devices, follow QOM tree for reset" or
what an API for that would look like).

thanks
-- PMM

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Markus Armbruster]

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

> On Fri, 24 May 2019 at 20:47, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted,
>> but qom devices not.
>
> It's not a qdev-vs-QOM thing. Anything which is a DeviceState
> has a reset method, but only devices which are somewhere
> rooted in the bus-tree that starts with the "main system
> bus" (aka sysbus) get reset by the vl.c-registered "reset
> everything on the system bus". Devices which are SysBusDevice
> get auto-parented onto the sysbus, and so get reset. Devices
> like PCI devices or SCSI devices get put onto the PCI
> bus or the SCSI bus, and those buses are in turn children
> of some host-controller device which is on the sysbus, so
> they all get reset. The things that don't get reset are
> "orphan" devices which are neither (a) of a type that gets
> automatically parented onto a bus like SysBusDevice nor
> (b) put specifically onto some other bus.
>
> CPU objects are the other common thing that doesn't get
> reset 'automatically'.
>
> Suggestions for how to restructure reset so this doesn't
> happen are welcome... "reset follows the bus hierarchy"
> works well in some places but is a bit weird in others
> (for SoC containers and the like "follow the QOM
> hierarchy" would make more sense, but I have no idea
> how to usefully transition to a model where you could
> say "for these devices, follow QOM tree for reset" or
> what an API for that would look like).

Here's a QOM composition tree for the ARM virt machine (-nodefaults
-device e1000) as visible in qom-fuse under /machine, with irq and
qemu:memory-region ommitted for brevity:

    machine  virt-4.1-machine
      +-- fw_cfg  fw_cfg_mem
      +-- peripheral  container
      +-- peripheral-anon  container
      |     +-- device[0]  e1000
      +-- unattached  container
      |     +-- device[0]  cortex-a15-arm-cpu
      |     +-- device[1]  arm_gic
      |     +-- device[2]  arm-gicv2m
      |     +-- device[3]  pl011
      |     +-- device[4]  pl031
      |     +-- device[5]  gpex-pcihost
      |     |     +-- pcie.0  PCIE
      |     |     +-- gpex_root  gpex-root
      |     +-- device[6]  pl061
      |     +-- device[7]  gpio-key
      |     +-- device[8]  virtio-mmio
      |     |     +-- virtio-mmio-bus.0  virtio-mmio-bus
      |     .
      |     .  more virtio-mmio
      |     .
      |     +-- device[39]  virtio-mmio
      |     |     +-- virtio-mmio-bus.31  virtio-mmio-bus
      |     +-- device[40]  platform-bus-device
      |     +-- sysbus  System
      +-- virt.flash0  cfi.pflash01
      +-- virt.flash1  cfi.pflash01

Observations:

* Some components of the machine are direct children of machine: fw_cfg,
  virt.flash0, virt.flash1

* machine additionally has a few containers: peripheral,
  peripheral-anon, unattached.

* machine/peripheral and machine/peripheral-anon contain the -device
  with and without ID, respectively.

* machine/unattached contains everything else created by code without an
  explicit parent device.  Some (all?) of them should perhaps be direct
  children of machine instead.

Compare to the qdev tree shown by info qtree:

    bus: main-system-bus
      type System
      dev: platform-bus-device, id "platform-bus-device"
      dev: fw_cfg_mem, id ""
      dev: virtio-mmio, id ""
        bus: virtio-mmio-bus.31
          type virtio-mmio-bus
      ... more virtio-mmio
      dev: virtio-mmio, id ""
        bus: virtio-mmio-bus.0
          type virtio-mmio-bus
      dev: gpio-key, id ""
      dev: pl061, id ""
      dev: gpex-pcihost, id ""
        bus: pcie.0
          type PCIE
          dev: e1000, id ""
          dev: gpex-root, id ""
      dev: pl031, id ""
      dev: pl011, id ""
      dev: arm-gicv2m, id ""
      dev: arm_gic, id ""
      dev: cfi.pflash01, id ""
      dev: cfi.pflash01, id ""

Observations:

* Composition tree root machine's containers are not in the qtree.

* Composition tree node cortex-a15-arm-cpu is not in the qtree.  That's
  because it's not a qdev (in QOM parlance: not a TYPE_DEVICE).

* In the qtree, every other inner node is a qbus.  These are *leaves* in
  the composition tree.  The qtree's vertex from qbus to qdev is a
  *link* in the composition tree.

  Example: main-system-bus -> pl011 is
      machine/unattached/sysbus/child[4] ->
      ../../../machine/unattached/device[3].

  Example: main-system-bus/gpex-pcihost/pcie.0 -> e1000 is
      machine/unattached/device[5]/pcie.0//child[1] ->
      ../../../../machine/peripheral-anon/device[0].

Now let me ramble a bit on reset.

We could model the reset wiring explicitly: every QOM object that wants
to participate in reset has a reset input pin.  We represent the wiring
as links.  The reset links form a reset tree.

Example: object virt-4.1-machine at machine gets a link reset[4]
    pointing to its child object cfi.pflash01 at machine/virt.flash0.

Example: object PCIE at machine/unattached/device[5]/pcie.0 gets a link
    reset[1] pointing to object e1000 at
    machine/peripheral-anon/device[0].

    Note that reset[1] points to the same object as child[1].

Adding all these links in code would be tiresome.  Enter defaults.

If an object doesn't get its reset input connected, and

* the parent is machine/peripheral or machine/peripheral-anon, default
  to the object that links to it in the composition tree.  Error when
  there's more than one.

  Example: object e1000 at machine/peripheral-anon/device[0] defaults to
  machine/unattached/device[5]/pcie.0.

* the parent is machine/unattached, default to machine.

  Example: object gpex-pcihost at machine/unattached/device[5] defaults
  to machine.

* the parent is not one of these containers, default to its parent in
  the composition tree.

  Example: object cfi.pflash01 at machine/virt.flash0 defaults to
  machine.

This leaves the order in which an object asserts its reset wires
unspecified.  In physical hardware, these guys get asserted
simultaneously, and the various pieces of silicon at the other ends
reset in parallel.  In software, we serialize.

We could pick some order by numbering the reset links, say reset[0],
reset[1], ...

Reset could then work by walking the reset tree, running device model
callbacks before and after walking the children.

Cases may well exist where reset is more complicated than that.  Perhaps
an object resets its children in the reset tree at different times
during its own reset.  Such cases need to modelled in device model code.
That's another callback.

This is quite possibly too naive to be practical.  Hey, you asked :)

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Philippe Mathieu-Daudé]

Cc'ing Damien who is working on "multi-phase reset mechanism".

On 5/27/19 9:52 AM, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Fri, 24 May 2019 at 20:47, Christian Borntraeger
>> <borntraeger@de.ibm.com> wrote:
>>> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted,
>>> but qom devices not.
>>
>> It's not a qdev-vs-QOM thing. Anything which is a DeviceState
>> has a reset method, but only devices which are somewhere
>> rooted in the bus-tree that starts with the "main system
>> bus" (aka sysbus) get reset by the vl.c-registered "reset
>> everything on the system bus". Devices which are SysBusDevice
>> get auto-parented onto the sysbus, and so get reset. Devices
>> like PCI devices or SCSI devices get put onto the PCI
>> bus or the SCSI bus, and those buses are in turn children
>> of some host-controller device which is on the sysbus, so
>> they all get reset. The things that don't get reset are
>> "orphan" devices which are neither (a) of a type that gets
>> automatically parented onto a bus like SysBusDevice nor
>> (b) put specifically onto some other bus.
>>
>> CPU objects are the other common thing that doesn't get
>> reset 'automatically'.
>>
>> Suggestions for how to restructure reset so this doesn't
>> happen are welcome... "reset follows the bus hierarchy"
>> works well in some places but is a bit weird in others
>> (for SoC containers and the like "follow the QOM
>> hierarchy" would make more sense, but I have no idea
>> how to usefully transition to a model where you could
>> say "for these devices, follow QOM tree for reset" or
>> what an API for that would look like).
> 
> Here's a QOM composition tree for the ARM virt machine (-nodefaults
> -device e1000) as visible in qom-fuse under /machine, with irq and
> qemu:memory-region ommitted for brevity:
> 
>     machine  virt-4.1-machine
>       +-- fw_cfg  fw_cfg_mem
>       +-- peripheral  container
>       +-- peripheral-anon  container
>       |     +-- device[0]  e1000
>       +-- unattached  container
>       |     +-- device[0]  cortex-a15-arm-cpu
>       |     +-- device[1]  arm_gic
>       |     +-- device[2]  arm-gicv2m
>       |     +-- device[3]  pl011
>       |     +-- device[4]  pl031
>       |     +-- device[5]  gpex-pcihost
>       |     |     +-- pcie.0  PCIE
>       |     |     +-- gpex_root  gpex-root
>       |     +-- device[6]  pl061
>       |     +-- device[7]  gpio-key
>       |     +-- device[8]  virtio-mmio
>       |     |     +-- virtio-mmio-bus.0  virtio-mmio-bus
>       |     .
>       |     .  more virtio-mmio
>       |     .
>       |     +-- device[39]  virtio-mmio
>       |     |     +-- virtio-mmio-bus.31  virtio-mmio-bus
>       |     +-- device[40]  platform-bus-device
>       |     +-- sysbus  System
>       +-- virt.flash0  cfi.pflash01
>       +-- virt.flash1  cfi.pflash01
> 
> Observations:
> 
> * Some components of the machine are direct children of machine: fw_cfg,
>   virt.flash0, virt.flash1
> 
> * machine additionally has a few containers: peripheral,
>   peripheral-anon, unattached.
> 
> * machine/peripheral and machine/peripheral-anon contain the -device
>   with and without ID, respectively.
> 
> * machine/unattached contains everything else created by code without an
>   explicit parent device.  Some (all?) of them should perhaps be direct
>   children of machine instead.
> 
> Compare to the qdev tree shown by info qtree:
> 
>     bus: main-system-bus
>       type System
>       dev: platform-bus-device, id "platform-bus-device"
>       dev: fw_cfg_mem, id ""
>       dev: virtio-mmio, id ""
>         bus: virtio-mmio-bus.31
>           type virtio-mmio-bus
>       ... more virtio-mmio
>       dev: virtio-mmio, id ""
>         bus: virtio-mmio-bus.0
>           type virtio-mmio-bus
>       dev: gpio-key, id ""
>       dev: pl061, id ""
>       dev: gpex-pcihost, id ""
>         bus: pcie.0
>           type PCIE
>           dev: e1000, id ""
>           dev: gpex-root, id ""
>       dev: pl031, id ""
>       dev: pl011, id ""
>       dev: arm-gicv2m, id ""
>       dev: arm_gic, id ""
>       dev: cfi.pflash01, id ""
>       dev: cfi.pflash01, id ""
> 
> Observations:
> 
> * Composition tree root machine's containers are not in the qtree.
> 
> * Composition tree node cortex-a15-arm-cpu is not in the qtree.  That's
>   because it's not a qdev (in QOM parlance: not a TYPE_DEVICE).
> 
> * In the qtree, every other inner node is a qbus.  These are *leaves* in
>   the composition tree.  The qtree's vertex from qbus to qdev is a
>   *link* in the composition tree.
> 
>   Example: main-system-bus -> pl011 is
>       machine/unattached/sysbus/child[4] ->
>       ../../../machine/unattached/device[3].
> 
>   Example: main-system-bus/gpex-pcihost/pcie.0 -> e1000 is
>       machine/unattached/device[5]/pcie.0//child[1] ->
>       ../../../../machine/peripheral-anon/device[0].
> 
> Now let me ramble a bit on reset.
> 
> We could model the reset wiring explicitly: every QOM object that wants
> to participate in reset has a reset input pin.  We represent the wiring
> as links.  The reset links form a reset tree.
> 
> Example: object virt-4.1-machine at machine gets a link reset[4]
>     pointing to its child object cfi.pflash01 at machine/virt.flash0.
> 
> Example: object PCIE at machine/unattached/device[5]/pcie.0 gets a link
>     reset[1] pointing to object e1000 at
>     machine/peripheral-anon/device[0].
> 
>     Note that reset[1] points to the same object as child[1].
> 
> Adding all these links in code would be tiresome.  Enter defaults.
> 
> If an object doesn't get its reset input connected, and
> 
> * the parent is machine/peripheral or machine/peripheral-anon, default
>   to the object that links to it in the composition tree.  Error when
>   there's more than one.
> 
>   Example: object e1000 at machine/peripheral-anon/device[0] defaults to
>   machine/unattached/device[5]/pcie.0.
> 
> * the parent is machine/unattached, default to machine.
> 
>   Example: object gpex-pcihost at machine/unattached/device[5] defaults
>   to machine.
> 
> * the parent is not one of these containers, default to its parent in
>   the composition tree.
> 
>   Example: object cfi.pflash01 at machine/virt.flash0 defaults to
>   machine.
> 
> This leaves the order in which an object asserts its reset wires
> unspecified.  In physical hardware, these guys get asserted
> simultaneously, and the various pieces of silicon at the other ends
> reset in parallel.  In software, we serialize.
> 
> We could pick some order by numbering the reset links, say reset[0],
> reset[1], ...
> 
> Reset could then work by walking the reset tree, running device model
> callbacks before and after walking the children.
> 
> Cases may well exist where reset is more complicated than that.  Perhaps
> an object resets its children in the reset tree at different times
> during its own reset.  Such cases need to modelled in device model code.
> That's another callback.
> 
> This is quite possibly too naive to be practical.  Hey, you asked :)

Thanks a lot for this extensive analysis :) I guess you indirectly
answered to some of the issues Peter pointed in
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg03667.html

Markus, if your bandwidth allows it, you might want to have a look at it :)

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Peter Maydell]

On Mon, 27 May 2019 at 08:52, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > Suggestions for how to restructure reset so this doesn't
> > happen are welcome... "reset follows the bus hierarchy"
> > works well in some places but is a bit weird in others
> > (for SoC containers and the like "follow the QOM
> > hierarchy" would make more sense, but I have no idea
> > how to usefully transition to a model where you could
> > say "for these devices, follow QOM tree for reset" or
> > what an API for that would look like).
>
> Here's a QOM composition tree for the ARM virt machine (-nodefaults
> -device e1000) as visible in qom-fuse under /machine, with irq and
> qemu:memory-region ommitted for brevity:

virt is a bit of an outlier because as a purely-virtual
machine it has no "SoC" -- it's just a bag of devices
at the machine level. It would be interesting to
also look at a machine that's emulating something
closer to real hardware (eg one of the aspeed machines,
or mps2-an521).

> Observations:
>
> * Composition tree root machine's containers are not in the qtree.
>
> * Composition tree node cortex-a15-arm-cpu is not in the qtree.  That's
>   because it's not a qdev (in QOM parlance: not a TYPE_DEVICE).

Hmm? The Arm CPUs all subclass CPUClass, which subclasses
DeviceState. The CPU is a qdev, but it is not in the qtree because
it does not have a bus it can live on.

> Now let me ramble a bit on reset.

Thanks for this -- I have put this on my list to
think through in detail next week.

-- PMM

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Markus Armbruster]

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

> On Mon, 27 May 2019 at 08:52, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > Suggestions for how to restructure reset so this doesn't
>> > happen are welcome... "reset follows the bus hierarchy"
>> > works well in some places but is a bit weird in others
>> > (for SoC containers and the like "follow the QOM
>> > hierarchy" would make more sense, but I have no idea
>> > how to usefully transition to a model where you could
>> > say "for these devices, follow QOM tree for reset" or
>> > what an API for that would look like).
>>
>> Here's a QOM composition tree for the ARM virt machine (-nodefaults
>> -device e1000) as visible in qom-fuse under /machine, with irq and
>> qemu:memory-region ommitted for brevity:
>
> virt is a bit of an outlier because as a purely-virtual
> machine it has no "SoC" -- it's just a bag of devices
> at the machine level. It would be interesting to
> also look at a machine that's emulating something
> closer to real hardware (eg one of the aspeed machines,
> or mps2-an521).

Can do.

>> Observations:
>>
>> * Composition tree root machine's containers are not in the qtree.
>>
>> * Composition tree node cortex-a15-arm-cpu is not in the qtree.  That's
>>   because it's not a qdev (in QOM parlance: not a TYPE_DEVICE).
>
> Hmm? The Arm CPUs all subclass CPUClass, which subclasses
> DeviceState. The CPU is a qdev, but it is not in the qtree because
> it does not have a bus it can live on.

You're right.

>> Now let me ramble a bit on reset.
>
> Thanks for this -- I have put this on my list to
> think through in detail next week.

Sounds good.

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Markus Armbruster]

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

> On Mon, 27 May 2019 at 08:52, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > Suggestions for how to restructure reset so this doesn't
>> > happen are welcome... "reset follows the bus hierarchy"
>> > works well in some places but is a bit weird in others
>> > (for SoC containers and the like "follow the QOM
>> > hierarchy" would make more sense, but I have no idea
>> > how to usefully transition to a model where you could
>> > say "for these devices, follow QOM tree for reset" or
>> > what an API for that would look like).
>>
>> Here's a QOM composition tree for the ARM virt machine (-nodefaults
>> -device e1000) as visible in qom-fuse under /machine, with irq and
>> qemu:memory-region ommitted for brevity:
>
> virt is a bit of an outlier because as a purely-virtual
> machine it has no "SoC" -- it's just a bag of devices
> at the machine level. It would be interesting to
> also look at a machine that's emulating something
> closer to real hardware (eg one of the aspeed machines,
> or mps2-an521).

Here you go: witherspoon-bmc (aspeed SoC) with -nodefaults and -device
m25p80 -device m25p80,id=qdev-id.  The -device are purely for
illustrating how user-plugged devices get added to the two trees.  I'm
not claiming they make sense.

QOM composition tree as visible in qom-fuse under /machine, with irq and
qemu:memory-region ommitted for brevity:

    machine  witherspoon-bmc-machine
      +-- peripheral  container
      |     +-- qdev-id  m25p80
      +-- peripheral-anon  container
      |     +-- device[0]  m25p80
      +-- soc  ast2500-a1
      |     +-- cpu  arm1176-arm-cpu
      |     +-- fmc  aspeed.smc.ast2500-fmc
      |     |     +-- spi  SSI
      |     +-- ftgmac100  ftgmac100
      |     +-- i2c  aspeed.i2c
      |     |     +-- aspeed.i2c.0  i2c-bus
      |     |     .
      |     |     .   more i2c-bus
      |     |     .
      |     |     +-- aspeed.i2c.13  i2c-bus
      |     +-- scu  aspeed.scu
      |     +-- sdmc  aspeed.sdmc
      |     +-- spi[0]  aspeed.smc.ast2500-spi1
      |     |     +-- spi  SSI
      |     +-- spi[1]  aspeed.smc.ast2500-spi2
      |     |     +-- spi  SSI
      |     +-- timerctrl  aspeed.timer
      |     +-- vic  aspeed.vic
      |     +-- wdt[0]  aspeed.wdt
      |     +-- wdt[1]  aspeed.wdt
      |     +-- wdt[2]  aspeed.wdt
      +-- unattached  container
            +-- device[0]  unimplemented-device
            +-- device[1]  mx25l25635e
            +-- device[2]  mx25l25635e
            +-- device[3]  mx66l1g45g
            +-- device[4]  pca9552
            +-- device[5]  tmp423
            +-- device[6]  tmp423
            +-- device[7]  tmp105
            +-- device[8]  ds1338
            +-- device[9]  smbus-eeprom
            +-- device[10]  pca9552
            +-- sysbus  System

Observations (same as for ARM virt, more or less):

* Where ARM virt had its onboard components as direct children of
  machine, witherspoon-bmc-machine has them wrapped in soc ast2500-a1.

* machine additionally has a few containers: peripheral,
  peripheral-anon, unattached.

* machine/peripheral and machine/peripheral-anon contain the -device
  with and without ID, respectively.

* machine/unattached contains everything else created by code without an
  explicit parent device.  Some (all?) of them should perhaps be direct
  children of machine or (unlike ARM virt) soc instead.

qdev tree shown by info qtree:

    bus: main-system-bus
      type System
      dev: unimplemented-device, id ""
        size = 2097152 (0x200000)
        name = "aspeed_soc.io"
        mmio 000000001e600000/0000000000200000
      dev: ftgmac100, id ""
        gpio-out "sysbus-irq" 1
        aspeed = true
        mac = "52:54:00:12:34:56"
        netdev = ""
        mmio 000000001e660000/0000000000002000
      dev: aspeed.wdt, id ""
        silicon-rev = 67175171 (0x4010303)
        mmio 000000001e785040/0000000000000020
      dev: aspeed.wdt, id ""
        silicon-rev = 67175171 (0x4010303)
        mmio 000000001e785020/0000000000000020
      dev: aspeed.wdt, id ""
        silicon-rev = 67175171 (0x4010303)
        mmio 000000001e785000/0000000000000020
      dev: aspeed.sdmc, id ""
        silicon-rev = 67175171 (0x4010303)
        ram-size = 536870912 (0x20000000)
        max-ram-size = 1073741824 (0x40000000)
        mmio 000000001e6e0000/0000000000001000
      dev: aspeed.smc.ast2500-spi2, id ""
        gpio-out "sysbus-irq" 2
        num-cs = 1 (0x1)
        mmio 000000001e631000/0000000000000100
        mmio 0000000038000000/0000000008000000
        bus: spi
          type SSI
          dev: m25p80, id "qdev-id"
            gpio-in "ssi-gpio-cs" 1
            nonvolatile-cfg = 36863 (0x8fff)
            spansion-cr1nv = 0 (0x0)
            spansion-cr2nv = 8 (0x8)
            spansion-cr3nv = 2 (0x2)
            spansion-cr4nv = 16 (0x10)
            drive = ""
          dev: m25p80, id ""
            gpio-in "ssi-gpio-cs" 1
            nonvolatile-cfg = 36863 (0x8fff)
            spansion-cr1nv = 0 (0x0)
            spansion-cr2nv = 8 (0x8)
            spansion-cr3nv = 2 (0x2)
            spansion-cr4nv = 16 (0x10)
            drive = ""
      dev: aspeed.smc.ast2500-spi1, id ""
        gpio-out "sysbus-irq" 2
        num-cs = 1 (0x1)
        mmio 000000001e630000/0000000000000100
        mmio 0000000030000000/0000000008000000
        bus: spi
          type SSI
          dev: mx66l1g45g, id ""
            gpio-in "ssi-gpio-cs" 1
            nonvolatile-cfg = 36863 (0x8fff)
            spansion-cr1nv = 0 (0x0)
            spansion-cr2nv = 8 (0x8)
            spansion-cr3nv = 2 (0x2)
            spansion-cr4nv = 16 (0x10)
            drive = ""
      dev: aspeed.smc.ast2500-fmc, id ""
        gpio-out "sysbus-irq" 3
        num-cs = 2 (0x2)
        mmio 000000001e620000/0000000000000100
        mmio 0000000020000000/0000000010000000
        bus: spi
          type SSI
          dev: mx25l25635e, id ""
            gpio-in "ssi-gpio-cs" 1
            nonvolatile-cfg = 36863 (0x8fff)
            spansion-cr1nv = 0 (0x0)
            spansion-cr2nv = 8 (0x8)
            spansion-cr3nv = 2 (0x2)
            spansion-cr4nv = 16 (0x10)
            drive = ""
          dev: mx25l25635e, id ""
            gpio-in "ssi-gpio-cs" 1
            nonvolatile-cfg = 36863 (0x8fff)
            spansion-cr1nv = 0 (0x0)
            spansion-cr2nv = 8 (0x8)
            spansion-cr3nv = 2 (0x2)
            spansion-cr4nv = 16 (0x10)
            drive = ""
      dev: aspeed.i2c, id ""
        gpio-out "sysbus-irq" 1
        mmio 000000001e78a000/0000000000001000
        bus: aspeed.i2c.13
          type i2c-bus
        ... more i2c-bus
        bus: aspeed.i2c.0
          type i2c-bus
      dev: aspeed.timer, id ""
        gpio-out "sysbus-irq" 8
        mmio 000000001e782000/0000000000001000
      dev: aspeed.vic, id ""
        gpio-out "sysbus-irq" 2
        gpio-in "" 51
        mmio 000000001e6c0000/0000000000020000
      dev: aspeed.scu, id ""
        silicon-rev = 67175171 (0x4010303)
        hw-strap1 = 4044018182 (0xf10ad206)
        hw-strap2 = 0 (0x0)
        hw-prot-key = 0 (0x0)
        mmio 000000001e6e2000/0000000000001000

Observations (same as for ARM virt):

* machine's containers are not in the qtree.

* Composition tree node arm1176-arm-cpu is not in the qtree.  That's
  because it isn't connected to a qbus.

  Same for pca9552, tmp423, tmp105, ds1338, smbus-eeprom, I guess.

* In the qtree, every other inner node is a qbus.  These are *leaves* in
  the composition tree.  The qtree's vertex from qbus to qdev is a
  *link* in the composition tree.

  Example: main-system-bus -> scu is
      machine/unattached/sysbus/child[0] ->
      ../../../machine/soc/scu.

  Example: main-system-bus -> unimplemented-device is
      machine/unattached/sysbus/child[12] ->
      ../../../machine/unattached/device[12].

  Example: main-system-bus/aspeed.smc.ast2500-spi1/spi -> mx66l1g45g is
      machine/soc/spi\[0\]/spi/child[0] ->
      ../../../../machine/unattached/device[3].

  Example: main-system-bus/aspeed.smc.ast2500-spi2/spi -> m25p80
      (the one without a qdev ID) is
      machine/soc/spi\[1\]/spi/child[0] ->
      ../../../../machine/peripheral-anon/device[0]

  Example: main-system-bus/aspeed.smc.ast2500-spi2/spi -> m25p80
      (the one with qdev ID "qdev-id") is
      machine/soc/spi\[1\]/spi/child[1] ->
      ../../../../machine/peripheral/qdev-id

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Philippe Mathieu-Daudé]

On 5/29/19 8:08 AM, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Mon, 27 May 2019 at 08:52, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>> Suggestions for how to restructure reset so this doesn't
>>>> happen are welcome... "reset follows the bus hierarchy"
>>>> works well in some places but is a bit weird in others
>>>> (for SoC containers and the like "follow the QOM
>>>> hierarchy" would make more sense, but I have no idea
>>>> how to usefully transition to a model where you could
>>>> say "for these devices, follow QOM tree for reset" or
>>>> what an API for that would look like).
>>>
>>> Here's a QOM composition tree for the ARM virt machine (-nodefaults
>>> -device e1000) as visible in qom-fuse under /machine, with irq and
>>> qemu:memory-region ommitted for brevity:
>>
>> virt is a bit of an outlier because as a purely-virtual
>> machine it has no "SoC" -- it's just a bag of devices
>> at the machine level. It would be interesting to
>> also look at a machine that's emulating something
>> closer to real hardware (eg one of the aspeed machines,
>> or mps2-an521).
> 
> Here you go: witherspoon-bmc (aspeed SoC) with -nodefaults and -device
> m25p80 -device m25p80,id=qdev-id.  The -device are purely for
> illustrating how user-plugged devices get added to the two trees.  I'm
> not claiming they make sense.
> 
> QOM composition tree as visible in qom-fuse under /machine, with irq and
> qemu:memory-region ommitted for brevity:
> 
>     machine  witherspoon-bmc-machine
>       +-- peripheral  container
>       |     +-- qdev-id  m25p80
>       +-- peripheral-anon  container
>       |     +-- device[0]  m25p80
>       +-- soc  ast2500-a1
>       |     +-- cpu  arm1176-arm-cpu
>       |     +-- fmc  aspeed.smc.ast2500-fmc
>       |     |     +-- spi  SSI
>       |     +-- ftgmac100  ftgmac100
>       |     +-- i2c  aspeed.i2c
>       |     |     +-- aspeed.i2c.0  i2c-bus
>       |     |     .
>       |     |     .   more i2c-bus
>       |     |     .
>       |     |     +-- aspeed.i2c.13  i2c-bus
>       |     +-- scu  aspeed.scu
>       |     +-- sdmc  aspeed.sdmc
>       |     +-- spi[0]  aspeed.smc.ast2500-spi1
>       |     |     +-- spi  SSI
>       |     +-- spi[1]  aspeed.smc.ast2500-spi2
>       |     |     +-- spi  SSI
>       |     +-- timerctrl  aspeed.timer
>       |     +-- vic  aspeed.vic
>       |     +-- wdt[0]  aspeed.wdt
>       |     +-- wdt[1]  aspeed.wdt
>       |     +-- wdt[2]  aspeed.wdt
>       +-- unattached  container
>             +-- device[0]  unimplemented-device
>             +-- device[1]  mx25l25635e
>             +-- device[2]  mx25l25635e
>             +-- device[3]  mx66l1g45g
>             +-- device[4]  pca9552
>             +-- device[5]  tmp423
>             +-- device[6]  tmp423
>             +-- device[7]  tmp105
>             +-- device[8]  ds1338
>             +-- device[9]  smbus-eeprom
>             +-- device[10]  pca9552
>             +-- sysbus  System
> 
> Observations (same as for ARM virt, more or less):
> 
> * Where ARM virt had its onboard components as direct children of
>   machine, witherspoon-bmc-machine has them wrapped in soc ast2500-a1.
> 
> * machine additionally has a few containers: peripheral,
>   peripheral-anon, unattached.
> 
> * machine/peripheral and machine/peripheral-anon contain the -device
>   with and without ID, respectively.
> 
> * machine/unattached contains everything else created by code without an
>   explicit parent device.  Some (all?) of them should perhaps be direct
>   children of machine or (unlike ARM virt) soc instead.
> 
> qdev tree shown by info qtree:
> 
>     bus: main-system-bus
>       type System
>       dev: unimplemented-device, id ""
>         size = 2097152 (0x200000)
>         name = "aspeed_soc.io"
>         mmio 000000001e600000/0000000000200000
>       dev: ftgmac100, id ""
>         gpio-out "sysbus-irq" 1
>         aspeed = true
>         mac = "52:54:00:12:34:56"
>         netdev = ""
>         mmio 000000001e660000/0000000000002000
>       dev: aspeed.wdt, id ""
>         silicon-rev = 67175171 (0x4010303)
>         mmio 000000001e785040/0000000000000020
>       dev: aspeed.wdt, id ""
>         silicon-rev = 67175171 (0x4010303)
>         mmio 000000001e785020/0000000000000020
>       dev: aspeed.wdt, id ""
>         silicon-rev = 67175171 (0x4010303)
>         mmio 000000001e785000/0000000000000020
>       dev: aspeed.sdmc, id ""
>         silicon-rev = 67175171 (0x4010303)
>         ram-size = 536870912 (0x20000000)
>         max-ram-size = 1073741824 (0x40000000)
>         mmio 000000001e6e0000/0000000000001000
>       dev: aspeed.smc.ast2500-spi2, id ""
>         gpio-out "sysbus-irq" 2
>         num-cs = 1 (0x1)
>         mmio 000000001e631000/0000000000000100
>         mmio 0000000038000000/0000000008000000
>         bus: spi
>           type SSI
>           dev: m25p80, id "qdev-id"
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>           dev: m25p80, id ""
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>       dev: aspeed.smc.ast2500-spi1, id ""
>         gpio-out "sysbus-irq" 2
>         num-cs = 1 (0x1)
>         mmio 000000001e630000/0000000000000100
>         mmio 0000000030000000/0000000008000000
>         bus: spi
>           type SSI
>           dev: mx66l1g45g, id ""
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>       dev: aspeed.smc.ast2500-fmc, id ""
>         gpio-out "sysbus-irq" 3
>         num-cs = 2 (0x2)
>         mmio 000000001e620000/0000000000000100
>         mmio 0000000020000000/0000000010000000
>         bus: spi
>           type SSI
>           dev: mx25l25635e, id ""
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>           dev: mx25l25635e, id ""
>             gpio-in "ssi-gpio-cs" 1
>             nonvolatile-cfg = 36863 (0x8fff)
>             spansion-cr1nv = 0 (0x0)
>             spansion-cr2nv = 8 (0x8)
>             spansion-cr3nv = 2 (0x2)
>             spansion-cr4nv = 16 (0x10)
>             drive = ""
>       dev: aspeed.i2c, id ""
>         gpio-out "sysbus-irq" 1
>         mmio 000000001e78a000/0000000000001000
>         bus: aspeed.i2c.13
>           type i2c-bus
>         ... more i2c-bus
>         bus: aspeed.i2c.0
>           type i2c-bus
>       dev: aspeed.timer, id ""
>         gpio-out "sysbus-irq" 8
>         mmio 000000001e782000/0000000000001000
>       dev: aspeed.vic, id ""
>         gpio-out "sysbus-irq" 2
>         gpio-in "" 51
>         mmio 000000001e6c0000/0000000000020000
>       dev: aspeed.scu, id ""
>         silicon-rev = 67175171 (0x4010303)
>         hw-strap1 = 4044018182 (0xf10ad206)
>         hw-strap2 = 0 (0x0)
>         hw-prot-key = 0 (0x0)
>         mmio 000000001e6e2000/0000000000001000
> 
> Observations (same as for ARM virt):
> 
> * machine's containers are not in the qtree.
> 
> * Composition tree node arm1176-arm-cpu is not in the qtree.  That's
>   because it isn't connected to a qbus.
> 
>   Same for pca9552, tmp423, tmp105, ds1338, smbus-eeprom, I guess.

That is odd:

static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
{
    AspeedSoCState *soc = &bmc->soc;

    i2c_create_slave(
        aspeed_i2c_get_bus(DEVICE(&soc->i2c), 3),
        TYPE_PCA9552, 0x60);
    ...

DeviceState *i2c_create_slave(I2CBus *bus, const char *name, ...
{
    DeviceState *dev;

    dev = qdev_create(&bus->qbus, name);
    ...

static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
{
    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
    AspeedI2CState *s = ASPEED_I2C(dev);

    for (i = 0; i < ASPEED_I2C_NR_BUSSES; i++) {
        s->busses[i].bus = i2c_init_bus(dev, name);
        ...

I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
{
    I2CBus *bus;

    bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name));
    ...

static const TypeInfo i2c_bus_info = {
    .name = TYPE_I2C_BUS,
    .parent = TYPE_BUS,
    ...

> * In the qtree, every other inner node is a qbus.  These are *leaves* in
>   the composition tree.  The qtree's vertex from qbus to qdev is a
>   *link* in the composition tree.
> 
>   Example: main-system-bus -> scu is
>       machine/unattached/sysbus/child[0] ->
>       ../../../machine/soc/scu.
> 
>   Example: main-system-bus -> unimplemented-device is
>       machine/unattached/sysbus/child[12] ->
>       ../../../machine/unattached/device[12].
> 
>   Example: main-system-bus/aspeed.smc.ast2500-spi1/spi -> mx66l1g45g is
>       machine/soc/spi\[0\]/spi/child[0] ->
>       ../../../../machine/unattached/device[3].
> 
>   Example: main-system-bus/aspeed.smc.ast2500-spi2/spi -> m25p80
>       (the one without a qdev ID) is
>       machine/soc/spi\[1\]/spi/child[0] ->
>       ../../../../machine/peripheral-anon/device[0]
> 
>   Example: main-system-bus/aspeed.smc.ast2500-spi2/spi -> m25p80
>       (the one with qdev ID "qdev-id") is
>       machine/soc/spi\[1\]/spi/child[1] ->
>       ../../../../machine/peripheral/qdev-id
> 

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[David Hildenbrand]

On 24.05.19 21:45, Christian Borntraeger wrote:
> 
> 
> On 24.05.19 21:00, David Hildenbrand wrote:
>> On 24.05.19 20:36, David Hildenbrand wrote:
>>> On 24.05.19 20:28, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 24.05.19 20:04, David Hildenbrand wrote:
>>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:
>>>>>> Hi Christian,
>>>>>>
>>>>>> I'm having hard time to understand why the S390_IPL object calls
>>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>>>>>> being QOM'ified (it has a reset method).
>>>>>>
>>>>>> It doesn't seem to have a qdev children added explicitly to it.
>>>>>> I see it is used as a singleton, what else am I missing?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Phil.
>>>>>>
>>>>>
>>>>> Looks like I added it back then (~4 years ago) when converting it into a
>>>>> TYPE_DEVICE.
>>>>>
>>>>> I could imagine that - back then - this was needed because only
>>>>> TYPE_SYS_BUS_DEVICE would recursively get reset.
>>>>
>>>> Yes, back then singleton devices were not recursively resetted. Has that changed?
>>>
>>> Hacking that call out, I don't see it getting called anymore. So it is
>>> still required. The question is if it can be reworked.
>>>
>>
>> Yes, as it is not a sysbus device, it won't get reset.
>> The owner (machine) has to take care of this. The following works:
>>
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index b93750c14e..91a31c2cd0 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>       */
>>      ipl->compat_start_addr = ipl->start_addr;
>>      ipl->compat_bios_start_addr = ipl->bios_start_addr;
>> -    qemu_register_reset(qdev_reset_all_fn, dev);
>>  error:
>>      error_propagate(errp, err);
>>  }
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index bbc6e8fa0b..658ab529a1 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
>>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>  }
>>  
>> +static void s390_ipl_reset(void)
>> +{
>> +    qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL)));
>> +}
>> +
>>  static void s390_machine_reset(void)
>>  {
>>      enum s390_reset reset_type;
>> @@ -353,6 +358,7 @@ static void s390_machine_reset(void)
>>      case S390_RESET_EXTERNAL:
>>      case S390_RESET_REIPL:
>>          qemu_devices_reset();
>> +        s390_ipl_reset();
>>          s390_crypto_reset();
>>  
>>          /* configure and start the ipl CPU only */
>>
> 
> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted,
> but qom devices not.
> 

Shall I send that as a proper patch, or do we want to stick to the
existing approach until we have improved the general reset approach?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Cornelia Huck]

On Tue, 28 May 2019 10:29:09 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 24.05.19 21:45, Christian Borntraeger wrote:
> > 
> > 
> > On 24.05.19 21:00, David Hildenbrand wrote:  
> >> On 24.05.19 20:36, David Hildenbrand wrote:  
> >>> On 24.05.19 20:28, Christian Borntraeger wrote:  
> >>>>
> >>>>
> >>>> On 24.05.19 20:04, David Hildenbrand wrote:  
> >>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:  
> >>>>>> Hi Christian,
> >>>>>>
> >>>>>> I'm having hard time to understand why the S390_IPL object calls
> >>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
> >>>>>> being QOM'ified (it has a reset method).
> >>>>>>
> >>>>>> It doesn't seem to have a qdev children added explicitly to it.
> >>>>>> I see it is used as a singleton, what else am I missing?
> >>>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>> Phil.
> >>>>>>  
> >>>>>
> >>>>> Looks like I added it back then (~4 years ago) when converting it into a
> >>>>> TYPE_DEVICE.
> >>>>>
> >>>>> I could imagine that - back then - this was needed because only
> >>>>> TYPE_SYS_BUS_DEVICE would recursively get reset.  
> >>>>
> >>>> Yes, back then singleton devices were not recursively resetted. Has that changed?  
> >>>
> >>> Hacking that call out, I don't see it getting called anymore. So it is
> >>> still required. The question is if it can be reworked.
> >>>  
> >>
> >> Yes, as it is not a sysbus device, it won't get reset.
> >> The owner (machine) has to take care of this. The following works:
> >>
> >>
> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> >> index b93750c14e..91a31c2cd0 100644
> >> --- a/hw/s390x/ipl.c
> >> +++ b/hw/s390x/ipl.c
> >> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> >>       */
> >>      ipl->compat_start_addr = ipl->start_addr;
> >>      ipl->compat_bios_start_addr = ipl->bios_start_addr;
> >> -    qemu_register_reset(qdev_reset_all_fn, dev);
> >>  error:
> >>      error_propagate(errp, err);
> >>  }
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index bbc6e8fa0b..658ab529a1 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
> >>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> >>  }
> >>  
> >> +static void s390_ipl_reset(void)
> >> +{
> >> +    qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL)));
> >> +}
> >> +
> >>  static void s390_machine_reset(void)
> >>  {
> >>      enum s390_reset reset_type;
> >> @@ -353,6 +358,7 @@ static void s390_machine_reset(void)
> >>      case S390_RESET_EXTERNAL:
> >>      case S390_RESET_REIPL:
> >>          qemu_devices_reset();
> >> +        s390_ipl_reset();
> >>          s390_crypto_reset();
> >>  
> >>          /* configure and start the ipl CPU only */
> >>  
> > 
> > While this patch is certainly ok, I find it disturbing that qdev devices are being resetted,
> > but qom devices not.
> >   
> 
> Shall I send that as a proper patch, or do we want to stick to the
> existing approach until we have improved the general reset approach?

I don't think the current code is really broken, so personally I'd
prefer to just leave it alone until we figured out how the reset should
work in general.

Re: [Qemu-devel] hw/s390x/ipl: Dubious use of qdev_reset_all_fn

[Philippe Mathieu-Daudé]

On 5/28/19 10:33 AM, Cornelia Huck wrote:
> On Tue, 28 May 2019 10:29:09 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 24.05.19 21:45, Christian Borntraeger wrote:
>>>
>>>
>>> On 24.05.19 21:00, David Hildenbrand wrote:  
>>>> On 24.05.19 20:36, David Hildenbrand wrote:  
>>>>> On 24.05.19 20:28, Christian Borntraeger wrote:  
>>>>>>
>>>>>>
>>>>>> On 24.05.19 20:04, David Hildenbrand wrote:  
>>>>>>> On 24.05.19 19:54, Philippe Mathieu-Daudé wrote:  
>>>>>>>> Hi Christian,
>>>>>>>>
>>>>>>>> I'm having hard time to understand why the S390_IPL object calls
>>>>>>>> qemu_register_reset(qdev_reset_all_fn) in its realize() method, while
>>>>>>>> being QOM'ified (it has a reset method).
>>>>>>>>
>>>>>>>> It doesn't seem to have a qdev children added explicitly to it.
>>>>>>>> I see it is used as a singleton, what else am I missing?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Phil.
>>>>>>>>  
>>>>>>>
>>>>>>> Looks like I added it back then (~4 years ago) when converting it into a
>>>>>>> TYPE_DEVICE.
>>>>>>>
>>>>>>> I could imagine that - back then - this was needed because only
>>>>>>> TYPE_SYS_BUS_DEVICE would recursively get reset.  
>>>>>>
>>>>>> Yes, back then singleton devices were not recursively resetted. Has that changed?  
>>>>>
>>>>> Hacking that call out, I don't see it getting called anymore. So it is
>>>>> still required. The question is if it can be reworked.
>>>>>  
>>>>
>>>> Yes, as it is not a sysbus device, it won't get reset.
>>>> The owner (machine) has to take care of this. The following works:
>>>>
>>>>
>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>>> index b93750c14e..91a31c2cd0 100644
>>>> --- a/hw/s390x/ipl.c
>>>> +++ b/hw/s390x/ipl.c
>>>> @@ -232,7 +232,6 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>>>       */
>>>>      ipl->compat_start_addr = ipl->start_addr;
>>>>      ipl->compat_bios_start_addr = ipl->bios_start_addr;
>>>> -    qemu_register_reset(qdev_reset_all_fn, dev);
>>>>  error:
>>>>      error_propagate(errp, err);
>>>>  }
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index bbc6e8fa0b..658ab529a1 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -338,6 +338,11 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
>>>>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>>>  }
>>>>  
>>>> +static void s390_ipl_reset(void)
>>>> +{
>>>> +    qdev_reset_all(DEVICE(object_resolve_path_type("", TYPE_S390_IPL, NULL)));
>>>> +}
>>>> +
>>>>  static void s390_machine_reset(void)
>>>>  {
>>>>      enum s390_reset reset_type;
>>>> @@ -353,6 +358,7 @@ static void s390_machine_reset(void)
>>>>      case S390_RESET_EXTERNAL:
>>>>      case S390_RESET_REIPL:
>>>>          qemu_devices_reset();
>>>> +        s390_ipl_reset();
>>>>          s390_crypto_reset();
>>>>  
>>>>          /* configure and start the ipl CPU only */
>>>>  
>>>
>>> While this patch is certainly ok, I find it disturbing that qdev devices are being resetted,
>>> but qom devices not.
>>>   
>>
>> Shall I send that as a proper patch, or do we want to stick to the
>> existing approach until we have improved the general reset approach?
> 
> I don't think the current code is really broken, so personally I'd
> prefer to just leave it alone until we figured out how the reset should
> work in general.

Agreed, I'd rather wait we better understand QOM/reset limitations, then
fix this properly, and finally kill the qdev_reset_all_fn() function.

Thanks all for having a look at this btw :)

Regards,

Phil.