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

[Damien Hedde]

Hi

On 6/18/19 6:13 PM, Peter Maydell wrote:
> 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.
> 
> 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
>