Re: [Qemu-arm] [RFC PATCH v2 00/12] Multi-phase reset

[Peter Maydell]

On Tue, 4 Jun 2019 at 17:25, Damien Hedde <address@hidden> 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