Re: [Qemu-devel] [PATCH 2/2] pseries: Use new hook to correct reset sequence

[David Gibson]

On Thu, Aug 02, 2012 at 02:40:19PM -0500, Anthony Liguori wrote:
> Andreas Färber <address@hidden> writes:
> 
> > Am 02.08.2012 20:29, schrieb Anthony Liguori:
> >> Andreas Färber <address@hidden> writes:
> >> 
> >>> Anthony was favoring moving reset code out of machines and expressed
> >>> dislike for looping through CPUs, which my above patch took into
> >>> account. The ordering issue between CPU and devices is still unsolved 
> >>> there.
> >>>
> >>> Some on-list comments from Anthony would be nice, since we are moving
> >>> into opposing directions here - having the sPAPR machine be more in
> >>> control vs. moving code away from the PC machine into target-i386 CPU
> >>> and/or common CPU code.
> >> 
> >> I already commented on the first patch because I had a feeling you'd
> >> post something like this ;-)
> >
> > I was not cc'ed. :(
> >
> >> Regarding reset:
> >> 
> >> 1) Devices should implement DeviceState::reset()
> >> 
> >> 2) If a device doesn't implement ::reset(), it should call
> >> qemu_register_reset()
> >> 
> >> 3) Reset should propagate through the device model, starting with the
> >> top-level machine which is logically what's plugged into the wall and
> >> is the source of power in the first place.
> >
> > So you changed your opinion over night?
> 
> No.
> 
> > I wanted to keep the reset callbacks in the machine. You applied a patch
> > breaking that pattern and argued you wanted to move reset code *out* of
> > the machine. Now you say the machine should *propagate* reset. Sorry,
> > that's unlogical to me...
> 
> You're not listening carefully.  Just a friendly piece of advise--
> instead of sending knee-jerk emails, spend some time going back and
> re-reading these discussions.
> 
> This has been discussed literally to death now for years.
> 
> Reset propagates.  There is unanimous consensus that this is the Right
> Way to model reset.  There is also wide consensus that reset typically
> will propagate through the composition tree although in some cases,
> reset actually propagates through the bus (this mostly affects devices
> that are children of /peripheral paths though).
> 
> The "root" of the composition tree is the machine.  The machine in the
> abstract sense, not the QEMUMachine sense.  QEMUMachine::init() should
> eventually become trivial--just create a handful of devices that
> represent the core components of the machine with everything else being
> created through composition.

So what code controls the order in which "the machine in the abstract
sense" initiates the reset at the top-leve?

> Open coded logic in QEMUMachine::init is always bad.  Handling reset for
> a specific device in QEMUMachine::init is bad.  That goes against the
> idea of making QEMUMachine::init trivial.
> 
> However, reset does logically start at QEMUMachine.  That doesn't mean
> that QEMUMachine should be explicitly resetting devices in a specific
> order.  This is why I was quick to comment on David's patch because the
> argument about having a controller that determines reset ordering was
> silly.  While this does exist on some architectures,

Some platforms; architecture does not imply a particular platform -
this is one of the more subtle and pervasive x86-isms around.

> it's not at all
> typical.

So?  If it sometimes exists, we need to support that model.  The
argument that "real" hardware never has reset order dependencies is
simply incorrect.

>  Reset should flow with QEMUMachine::reset just playing the
> role of deciding whether it starts propagating from.
> 
> The only machines that can have complex reset logic are ones that can
> afford to take an extremely long time to startup--typically doing a
> tremendous amount of self-checks in the process.  These are not common
> among the types of machines QEMU simulates.

"having at least one order dependency in reset" != "complex and slow
reset logic".

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson