[Qemu-devel] [RFC PATCH v2 00/12] Multi-phase reset

Damien Hedde posted 12 patches 4 years, 9 months ago
Test s390x passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190604162526.10655-1-damien.hedde@greensocs.com
Maintainers: Cornelia Huck <cohuck@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Dmitry Fleytman <dmitry.fleytman@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Gerd Hoffmann <kraxel@redhat.com>, Alistair Francis <alistair@alistair23.me>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Halil Pasic <pasic@linux.ibm.com>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, Richard Henderson <rth@twiddle.net>, David Hildenbrand <david@redhat.com>, Fam Zheng <fam@euphon.net>, Eduardo Habkost <ehabkost@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Collin Walling <walling@linux.ibm.com>, John Snow <jsnow@redhat.com>
docs/devel/reset.txt           | 151 +++++++++
hw/arm/xilinx_zynq.c           |  14 +-
hw/audio/intel-hda.c           |   2 +-
hw/char/cadence_uart.c         |  81 ++++-
hw/core/Makefile.objs          |   2 +
hw/core/bus.c                  |  60 ++++
hw/core/qdev-vmstate.c         |  34 +++
hw/core/qdev.c                 | 124 +++++++-
hw/core/resettable.c           | 121 ++++++++
hw/hyperv/hyperv.c             |   2 +-
hw/i386/pc.c                   |   2 +-
hw/ide/microdrive.c            |   8 +-
hw/intc/spapr_xive.c           |   2 +-
hw/misc/zynq_slcr.c            | 543 ++++++++++++++++++---------------
hw/ppc/pnv_psi.c               |   2 +-
hw/ppc/spapr_pci.c             |   2 +-
hw/ppc/spapr_vio.c             |   2 +-
hw/s390x/s390-pci-inst.c       |   2 +-
hw/scsi/vmw_pvscsi.c           |   2 +-
hw/sd/omap_mmc.c               |   2 +-
hw/sd/pl181.c                  |   2 +-
include/hw/char/cadence_uart.h |  10 +-
include/hw/qdev-core.h         | 112 ++++++-
include/hw/resettable.h        | 104 +++++++
tests/Makefile.include         |   1 +
25 files changed, 1105 insertions(+), 282 deletions(-)
create mode 100644 docs/devel/reset.txt
create mode 100644 hw/core/qdev-vmstate.c
create mode 100644 hw/core/resettable.c
create mode 100644 include/hw/resettable.h
[Qemu-devel] [RFC PATCH v2 00/12] Multi-phase reset
Posted by Damien Hedde 4 years, 9 months ago
Hi all,

Here's the second version of the multi-phase reset proposal patches.

# DESCRIPTION

Basically the reset procedure is split in 3 parts:
INIT PHASE: Reset the object internal state, put a resetting flag and do the
    same for the reset subtree. No side effect on other devices to guarantee
    that, in a reset domain, everything get initialized first. This corresponds
    mostly to what is currently done in the device/bus reset method.

HOLD PHASE: This phase allows to control a reset with a IO. When a IO control
    reset procedure based on the IO level (not edge), we may need to assert
    the reset, wait some time, and finally de-assert the reset. The consequence
    is that such a device can stay in a "resetting state" and may need to show
    this state to other devices through its outputs. For example, a clock
    controller will typically shutdown its clocks when it is in resetting state.

EXIT PHASE: This phase sets outputs to state after reset. For a clock controller
     it starts the clocks. It also clears the "resetting" flag. A device should
     not react to inputs until this flag has been cleared. During this phase,
     outputs are propagated.

The Resettable interface is detailed in the added doc in patch 7.

# CHANGE SINCE V1

The series now focus only on the Resettable interface (no more ResetDomain).
Proposed changed have been applied:
 - reset_count getter/modifier methods
 - a foreach_child method

This last method allows some flexibility on what is the hierarchy of reset.
There is some discussion ongoing about this point. Althought the series does
not aim to modify the qdev/qbus-centric reset behavior, it is not fixed. An
object specialization can override it.

# RESET DEPRECATION

There is 3 changes in the current way of handling reset (for users or
developers of Devices)

1. qdev/qbus_reset_all

Theses functions are deprecated by this series and should be replaced by
direct call to device_reset or bus_reset. There is only a few existing calls
so I can probably replace them all and delete the original functions.

2. old's device_reset

There is a few call to this function, I renamed it *device_legacy_reset* to
handle the transition. This function allowed to reset only a given device 
(and not its eventual qbus subtree). This behavior is not possible with
the Resettable interface. At first glance it seemed that it is used only on 
device having no sub-buses so we could just use the new device_reset.
But I need to look at them more closely to be sure. If this behavior is really
needed (but why would we not reset children ?), it's possible to do a specific
function for Device to to 3-phases reset without the children.

3. DeviceClass's reset and BusClass's methods

This is the major change. The method is deprecated because reset methods are
now located in the interface class. In the series, I make the init phase
redirect to the original reset method of DeviceClass (or BusClass). There a
lot of use of the method and transitioning to 3-phases only reset will need
some work.

# MIGRATION

Bus reset state migration is right now not handled.

Regarding "migration-during-reset should Just Work, without necessarily
needing any specific changes for a device". The included patch define a vmsd
subsection which must to be added to every device main vmsd structure for
migration-during-reset of theses devices to work. I tried to find a way to
avoid that but really don't see how to achieve that.

So in the current state of this series, migration can only be supported for
leaf device (in term of qdev/qbus) where we add the subsection.

I'm not sure the migration is the problem here. I doubt a device will
support staying in reset state (which is a new feature) without manual
intervention. So migration of this unsupported state without any specific
change may not be a real asset.

The series is organised as follow:
 - Patch   1 adds Resettable interface
 - Patches 2 and 3 rename device_reset function by device_legacy_reset to prepare
   the transition.
 - Patches 4 to 6 make the changes in Device and Bus classes. 
 - Patches 7 add some doc
 - Patches 8 to 12 modify the xilinx_zynq to add 3-phases reset support in the
   uart and the slcr (the reset controller).

Thank you for your feedback,
Damien

Damien Hedde (12):
  Create Resettable QOM interface
  add device_legacy_reset function to do the transition with
    device_reset
  replace all occurences of device_reset by device_legacy_reset
  make Device and Bus Resettable
  Add function to control reset with gpio inputs
  add vmstate description for device reset state
  add doc about Resettable interface
  hw/misc/zynq_slcr: use standard register definition
  convert cadence_uart to 3-phases reset
  Convert zynq's slcr to 3-phases reset
  Add uart reset support in zynq_slcr
  Connect the uart reset gpios in the zynq platform

 docs/devel/reset.txt           | 151 +++++++++
 hw/arm/xilinx_zynq.c           |  14 +-
 hw/audio/intel-hda.c           |   2 +-
 hw/char/cadence_uart.c         |  81 ++++-
 hw/core/Makefile.objs          |   2 +
 hw/core/bus.c                  |  60 ++++
 hw/core/qdev-vmstate.c         |  34 +++
 hw/core/qdev.c                 | 124 +++++++-
 hw/core/resettable.c           | 121 ++++++++
 hw/hyperv/hyperv.c             |   2 +-
 hw/i386/pc.c                   |   2 +-
 hw/ide/microdrive.c            |   8 +-
 hw/intc/spapr_xive.c           |   2 +-
 hw/misc/zynq_slcr.c            | 543 ++++++++++++++++++---------------
 hw/ppc/pnv_psi.c               |   2 +-
 hw/ppc/spapr_pci.c             |   2 +-
 hw/ppc/spapr_vio.c             |   2 +-
 hw/s390x/s390-pci-inst.c       |   2 +-
 hw/scsi/vmw_pvscsi.c           |   2 +-
 hw/sd/omap_mmc.c               |   2 +-
 hw/sd/pl181.c                  |   2 +-
 include/hw/char/cadence_uart.h |  10 +-
 include/hw/qdev-core.h         | 112 ++++++-
 include/hw/resettable.h        | 104 +++++++
 tests/Makefile.include         |   1 +
 25 files changed, 1105 insertions(+), 282 deletions(-)
 create mode 100644 docs/devel/reset.txt
 create mode 100644 hw/core/qdev-vmstate.c
 create mode 100644 hw/core/resettable.c
 create mode 100644 include/hw/resettable.h

-- 
2.21.0


Re: [Qemu-devel] [RFC PATCH v2 00/12] Multi-phase reset
Posted by Peter Maydell 4 years, 9 months ago
On Tue, 4 Jun 2019 at 17:25, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Hi all,
>
> Here's the second version of the multi-phase reset proposal patches.

Sorry for leaving this one hanging again. Some initial general
comments...

> # DESCRIPTION
>
> Basically the reset procedure is split in 3 parts:
> INIT PHASE: Reset the object internal state, put a resetting flag and do the
>     same for the reset subtree. No side effect on other devices to guarantee
>     that, in a reset domain, everything get initialized first. This corresponds
>     mostly to what is currently done in the device/bus reset method.
>
> HOLD PHASE: This phase allows to control a reset with a IO. When a IO control
>     reset procedure based on the IO level (not edge), we may need to assert
>     the reset, wait some time, and finally de-assert the reset. The consequence
>     is that such a device can stay in a "resetting state" and may need to show
>     this state to other devices through its outputs. For example, a clock
>     controller will typically shutdown its clocks when it is in resetting state.
>
> EXIT PHASE: This phase sets outputs to state after reset. For a clock controller
>      it starts the clocks. It also clears the "resetting" flag. A device should
>      not react to inputs until this flag has been cleared. During this phase,
>      outputs are propagated.
>
> The Resettable interface is detailed in the added doc in patch 7.
>
> # CHANGE SINCE V1
>
> The series now focus only on the Resettable interface (no more ResetDomain).
> Proposed changed have been applied:
>  - reset_count getter/modifier methods
>  - a foreach_child method
>
> This last method allows some flexibility on what is the hierarchy of reset.
> There is some discussion ongoing about this point. Althought the series does
> not aim to modify the qdev/qbus-centric reset behavior, it is not fixed. An
> object specialization can override it.

I've looked through the patches, and I think my current concerns
(stated briefly) are:
 * I don't think we have the "don't call device implementations of
   reset phase functions multiple times" semantics that I wanted;
   OTOH I think the logic I suggested for those in comments on the
   v1 of this series wouldn't work either.
 * handling of "call the parent class's reset" is not very clean
   (but this is a general QOM design issue)
 * migration (see below)

> # RESET DEPRECATION
>
> There is 3 changes in the current way of handling reset (for users or
> developers of Devices)
>
> 1. qdev/qbus_reset_all
>
> Theses functions are deprecated by this series and should be replaced by
> direct call to device_reset or bus_reset. There is only a few existing calls
> so I can probably replace them all and delete the original functions.

Sounds good.

> 2. old's device_reset
>
> There is a few call to this function, I renamed it *device_legacy_reset* to
> handle the transition. This function allowed to reset only a given device
> (and not its eventual qbus subtree). This behavior is not possible with
> the Resettable interface. At first glance it seemed that it is used only on
> device having no sub-buses so we could just use the new device_reset.
> But I need to look at them more closely to be sure. If this behavior is really
> needed (but why would we not reset children ?), it's possible to do a specific
> function for Device to to 3-phases reset without the children.

Users of this function:
 hw/audio/intel-hda.c
  -- used by the HDA device to reset the HDACodecDevice, which has
     no child buses
 hw/hyperv/hyperv.c
  -- resets the SynICState, which I think has no child buses
 hw/i386/pc.c
  -- resets the APIC, which has no child buses. (This reset is only
     done as a workaround for lack of reset phases: the whole machine
     is reset and then the APIC is re-reset last to undo any changes
     that other devices might have made to it. Probably making the APIC
     support phased reset would allow us to drop this hack.)
 hw/ide/microdrive.c
  -- called here to reset the MicroDriveState object. This does have
     a child bus, but md_reset() explicitly calls ide_bus_reset(),
     so it should be possible to clean this up.
 hw/intc/spapr_xive.c
  -- resets the SpaprXive device, which I think has no child buses
 hw/ppc/pnv_psi.c
  -- resets a XiveSource, which I think has no child buses
 hw/ppc/spapr_pci.c
  -- resets every QOM child of the SpaprPhbState. This one will
     require closer checking, but my guess is that if there's a child
     with a child bus then failure to reset the bus would be a bug.
 hw/ppc/spapr_vio.c
  -- resets an SpaprTceTable. This needs attention from an Spapr
     expert but is probably ok.
 hw/s390x/s390-pci-inst.c
  -- resets the S390PCIBusDevice. Needs S390 expertise, but probably
     either no child buses or failure to reset them is a bug.
 hw/scsi/vmw_pvscsi.c
  -- resets an individual SCSIDevice. I don't think those have child buses.
 hw/sd/omap_mmc.c
  -- resetting an SDState, which has no child bus
 hw/sd/pl181.c
  -- ditto.

So there are one or two not-totally-trivial cases but we should
be able to deal with these.

> 3. DeviceClass's reset and BusClass's methods
>
> This is the major change. The method is deprecated because reset methods are
> now located in the interface class. In the series, I make the init phase
> redirect to the original reset method of DeviceClass (or BusClass). There a
> lot of use of the method and transitioning to 3-phases only reset will need
> some work.

I think it should be possible to do the conversion mechanically
(eg with coccinelle). We can look at doing that later.

> # MIGRATION
>
> Bus reset state migration is right now not handled.
>
> Regarding "migration-during-reset should Just Work, without necessarily
> needing any specific changes for a device". The included patch define a vmsd
> subsection which must to be added to every device main vmsd structure for
> migration-during-reset of theses devices to work. I tried to find a way to
> avoid that but really don't see how to achieve that.
>
> So in the current state of this series, migration can only be supported for
> leaf device (in term of qdev/qbus) where we add the subsection.
>
> I'm not sure the migration is the problem here. I doubt a device will
> support staying in reset state (which is a new feature) without manual
> intervention. So migration of this unsupported state without any specific
> change may not be a real asset.

The approach I thought would be good turns out not to actually work,
so I need to think a bit more about this.

I think what I would like to achieve is "default to doing the right
thing" -- ideally devices that add support for being held in reset
should not need to manually do something to make the bus/device reset
state be migrated properly. Otherwise we have an easy mistake to
make (forgetting to do the necessary handling of migration) when
adding a new device or making a device support being held in reset.

Regarding "devices not supporting staying in reset state without
manual intervention" do you mean that they might not correctly
deal with incoming signals or guest register write attempts
in the held-in-reset state? That's true, but I don't think it's
too terrible. (In particular it's a bug that can be fixed without
breaking migration compatibility; and it seems unlikely that guest
software would be trying to read or write a device that it's put
into reset.)

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v2 00/12] Multi-phase reset
Posted by Damien Hedde 4 years, 9 months ago
Hi

On 6/18/19 6:13 PM, Peter Maydell wrote:
> On Tue, 4 Jun 2019 at 17:25, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Hi all,
>>
>> Here's the second version of the multi-phase reset proposal patches.
> 
> I've looked through the patches, and I think my current concerns
> (stated briefly) are:
>  * I don't think we have the "don't call device implementations of
>    reset phase functions multiple times" semantics that I wanted;
>    OTOH I think the logic I suggested for those in comments on the
>    v1 of this series wouldn't work either.

I see now what you meant. I think it is possible but the cold/warm make
it more complicated. It will need a "get_reset_kind" method which
returns whether it's cold or warm.

>  * handling of "call the parent class's reset" is not very clean
>    (but this is a general QOM design issue)

Is it the current good way of doing this ? (some kind of
override_this_parent_method function)

>  * migration (see below)
> 
>> # RESET DEPRECATION
>>
>> There is 3 changes in the current way of handling reset (for users or
>> developers of Devices)
>>
>> 1. qdev/qbus_reset_all
>>
>> Theses functions are deprecated by this series and should be replaced by
>> direct call to device_reset or bus_reset. There is only a few existing calls
>> so I can probably replace them all and delete the original functions.
> 
> Sounds good.
> 
>> 2. old's device_reset
>>
>> There is a few call to this function, I renamed it *device_legacy_reset* to
>> handle the transition. This function allowed to reset only a given device
>> (and not its eventual qbus subtree). This behavior is not possible with
>> the Resettable interface. At first glance it seemed that it is used only on
>> device having no sub-buses so we could just use the new device_reset.
>> But I need to look at them more closely to be sure. If this behavior is really
>> needed (but why would we not reset children ?), it's possible to do a specific
>> function for Device to to 3-phases reset without the children.
> 
> Users of this function:
>  hw/audio/intel-hda.c
>   -- used by the HDA device to reset the HDACodecDevice, which has
>      no child buses
>  hw/hyperv/hyperv.c
>   -- resets the SynICState, which I think has no child buses
>  hw/i386/pc.c
>   -- resets the APIC, which has no child buses. (This reset is only
>      done as a workaround for lack of reset phases: the whole machine
>      is reset and then the APIC is re-reset last to undo any changes
>      that other devices might have made to it. Probably making the APIC
>      support phased reset would allow us to drop this hack.)
>  hw/ide/microdrive.c
>   -- called here to reset the MicroDriveState object. This does have
>      a child bus, but md_reset() explicitly calls ide_bus_reset(),
>      so it should be possible to clean this up.
>  hw/intc/spapr_xive.c
>   -- resets the SpaprXive device, which I think has no child buses
>  hw/ppc/pnv_psi.c
>   -- resets a XiveSource, which I think has no child buses
>  hw/ppc/spapr_pci.c
>   -- resets every QOM child of the SpaprPhbState. This one will
>      require closer checking, but my guess is that if there's a child
>      with a child bus then failure to reset the bus would be a bug.
>  hw/ppc/spapr_vio.c
>   -- resets an SpaprTceTable. This needs attention from an Spapr
>      expert but is probably ok.
>  hw/s390x/s390-pci-inst.c
>   -- resets the S390PCIBusDevice. Needs S390 expertise, but probably
>      either no child buses or failure to reset them is a bug.
>  hw/scsi/vmw_pvscsi.c
>   -- resets an individual SCSIDevice. I don't think those have child buses.
>  hw/sd/omap_mmc.c
>   -- resetting an SDState, which has no child bus
>  hw/sd/pl181.c
>   -- ditto.
> 
> So there are one or two not-totally-trivial cases but we should
> be able to deal with these.

Thanks for listing theses. Do you think I should includes all the
switches in the series or just the trivial ones ?

> 
>> 3. DeviceClass's reset and BusClass's methods
>>
>> This is the major change. The method is deprecated because reset methods are
>> now located in the interface class. In the series, I make the init phase
>> redirect to the original reset method of DeviceClass (or BusClass). There a
>> lot of use of the method and transitioning to 3-phases only reset will need
>> some work.
> 
> I think it should be possible to do the conversion mechanically
> (eg with coccinelle). We can look at doing that later.
> 
>> # MIGRATION
>>
>> Bus reset state migration is right now not handled.
>>
>> Regarding "migration-during-reset should Just Work, without necessarily
>> needing any specific changes for a device". The included patch define a vmsd
>> subsection which must to be added to every device main vmsd structure for
>> migration-during-reset of theses devices to work. I tried to find a way to
>> avoid that but really don't see how to achieve that.
>>
>> So in the current state of this series, migration can only be supported for
>> leaf device (in term of qdev/qbus) where we add the subsection.
>>
>> I'm not sure the migration is the problem here. I doubt a device will
>> support staying in reset state (which is a new feature) without manual
>> intervention. So migration of this unsupported state without any specific
>> change may not be a real asset.
> 
> The approach I thought would be good turns out not to actually work,
> so I need to think a bit more about this.
> 
> I think what I would like to achieve is "default to doing the right
> thing" -- ideally devices that add support for being held in reset
> should not need to manually do something to make the bus/device reset
> state be migrated properly. Otherwise we have an easy mistake to
> make (forgetting to do the necessary handling of migration) when
> adding a new device or making a device support being held in reset.

To have a default behavior, we need to somehow handle that in the
DeviceClass.
Ideally speaking, this subsection could be handled during the device
realization, when the vmsd is registered. But since we cannot
dynamically add a new subsection to the device vmsd structure, we have
to forge an extended new one (the same with additional subsection). It
seems technically possible but this feels like a bad hacky idea.

> 
> Regarding "devices not supporting staying in reset state without
> manual intervention" do you mean that they might not correctly
> deal with incoming signals or guest register write attempts
> in the held-in-reset state? That's true, but I don't think it's
> too terrible. (In particular it's a bug that can be fixed without
> breaking migration compatibility; and it seems unlikely that guest
> software would be trying to read or write a device that it's put
> into reset.)

Yes it is what I meant.

> 
> thanks
> -- PMM
>