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

[Anthony Liguori <anthony@codemonkey.ws>]

Andreas Färber <afaerber@suse.de> writes:

> Am 02.08.2012 20:29, schrieb Anthony Liguori:
>> Andreas Färber <afaerber@suse.de> 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.

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, it's not at all
typical.  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.

Regards,

Anthony Liguori

>
> If the machine should propagate reset then the disputed i386 patch is
> doing The Wrong Thing.
>
> If reset code should vanish from machine code AFAP then this patch is
> doing The Wrong Thing.
>
> No?
>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg