From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzGQ9-0004qJ-7d for qemu-devel@nongnu.org; Wed, 08 Aug 2012 20:16:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SzGQ7-0000fu-J7 for qemu-devel@nongnu.org; Wed, 08 Aug 2012 20:16:13 -0400 Received: from ozlabs.org ([203.10.76.45]:54657) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzGQ6-0000fd-Ug for qemu-devel@nongnu.org; Wed, 08 Aug 2012 20:16:11 -0400 Date: Thu, 9 Aug 2012 10:12:43 +1000 From: David Gibson Message-ID: <20120809001243.GA14655@truffala.fritz.box> References: <1343873409-8571-3-git-send-email-david@gibson.dropbear.id.au> <501AA071.3030406@suse.de> <87vch1i1va.fsf@codemonkey.ws> <501AC915.5080004@suse.de> <87lihx84m4.fsf@codemonkey.ws> <501BE7CE.4080200@suse.de> <1344376938.2698.27.camel@pasglop> <50219787.2000407@suse.de> <20120808014516.GQ16664@truffala.fritz.box> <50228423.7060003@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <50228423.7060003@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] pseries: Use new hook to correct reset sequence List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: qemu-devel@nongnu.org, agraf@suse.de, Anthony Liguori , Igor Mammedov On Wed, Aug 08, 2012 at 05:22:11PM +0200, Andreas F=E4rber wrote: > Am 08.08.2012 03:45, schrieb David Gibson: > > On Wed, Aug 08, 2012 at 12:32:39AM +0200, Andreas F=E4rber wrote: > >> Am 08.08.2012 00:02, schrieb Benjamin Herrenschmidt: > >>> On Fri, 2012-08-03 at 17:01 +0200, Andreas F=E4rber wrote: > >>>> > >>>> I have posted a suggestion where CPU reset is triggered by "the > >>>> machine > >>>> as an abstract concept" (needs a bit of tweaking still, but the > >>>> general > >>>> idea is there). > >>>> Based on that, shouldn't it be rather easy to add a Notifier simil= ar > >>>> to > >>>> "machine init done" that lets individual machines do post-reset se= tup? > >>>> I.e. not have QEMUMachine trigger and control the reset. > >>>> > >>> > >>> Note that we really want pre and post reset vs the device reset. > >>> > >>> That's why the machine should be the one in charge. The top level o= f the > >>> reset sequencing is -not- the CPU, it's the machine. All machines (= or > >>> SoCs) have some kind of reset controller and provide facilities for > >>> resetting individual devices, busses, processor cores.... the globa= l > >>> "system" reset (when it exists) itself might have interesting order= ing > >>> or sequencing requirements. > >>> > >>> Now, to fix our immediate problem on ppc for 1.2 the hook proposed = by > >>> Anthony for which David sent a patch does the job just fine, it all= ows > >>> us to clean out all our iommu tables before the device-reset, meani= ng > >>> that in-flights DMA cannot overwrite the various "files" (SLOF imag= e > >>> etc.... that are auto-loaded via reset handlers implicitely created= by > >>> load_image_targphys), and we can then do some post-initializations = as > >>> well to get things ready for a restart (rebuild the device-tree, et= c...) > >> > >> That's all good, except for embedded machines without such implicit > >> reset handling. It does contradict the "a machine is just a config f= ile, > >> setting up QOM objects" concept, but I was not the one to push that!= :) > >> > >> What I was thinking about however were those mentioned individual co= res > >> being reset using cpu_reset(). If we want to piggy-back some > >> machine-specific register initialization for individual CPUStates th= en > >> QEMUMachine::reset is not going to be enough because it only gets > >> triggered for complete system reset. My suggestion was thus to just = call > >> cpu_reset() in your QEMUMachine::reset and have cpu_reset() take car= e of > >> its initialization wherever called from. Any of these solutions are = easy > >> to implement for 1.2 if agreement is reached what people want. > >=20 > > So, I more or less reaslied that myself and my new version of the > > reset patch (which I expect to send out later today) kind of does > > that. I no longer do the machine specific CPU state setup from the > > QEMUMachine::reset, it's done from the per-cpu reset handler. The > > QEMUMachine::reset just does the special setup that's only for the > > CPU0 entry conditions, which *is* specific to a full system reset (no= t > > that I think we can get an individual CPU reset on pseries, anyway). > >=20 > >> What I am missing from Anthony's side is some communication to machi= ne > >> maintainers on the course to adopt before applying random patches. R= ight > >> now x86 and ppc are moving into opposite directions and arm, mips, e= tc. > >> maintainers may not even be aware of ongoing changes, and there's a > >> pending uc32 machine that should be reviewed in this light. > >=20 > > So.. having the CPU reset at the top of the tree definitely makes no > > sense - if nothing else, *which* cpu when there's more than one. >=20 > Maybe let me restate clearly what I am looking for in this discussion: >=20 > I would like a clear definition of > * what is the "normal" case, and > * what is the special case. >=20 > The special case sPAPR seems uncontroversial. >=20 > So, a bonus would be if we can have a default implementation (of > QEMUMachine::reset or whatever we end up doing) so that the average > machine does not need to fiddle with reset callbacks in > QEMUMachine::init. For example, have a machine_default_reset() as > fallback for QEMUMachine::reset =3D=3D NULL that resets all CPUs (in or= der > of the singly linked list) and then does qemu_devices_reset()? sPAPR > would then override that default implementation by specifying its own > implementation and we could get rid of reset callbacks in an estimated > 70% of QEMUMachine::init. (The less people fiddle at that level the > easier to refactor for me.) That could well be a later follow-up to you= r > v2, which looked okay on brief sight. We already have that. If QEMUMachine::reset is NULL, qemu_system_reset() does qemu_devices_reset() which is exactly the same as what it did before. qemu_devices_reset() calls all the reset callback handlers, so it will also reset the CPUs if a suitable CPU reset handler has been registered. --=20 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