From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53443) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Swf9k-0005Ow-B1 for qemu-devel@nongnu.org; Wed, 01 Aug 2012 16:04:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Swf9f-0002Xb-QB for qemu-devel@nongnu.org; Wed, 01 Aug 2012 16:04:32 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:40366) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Swf9f-0002XV-J3 for qemu-devel@nongnu.org; Wed, 01 Aug 2012 16:04:27 -0400 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 1 Aug 2012 14:04:26 -0600 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 2CE0E1FF001F for ; Wed, 1 Aug 2012 20:03:44 +0000 (WET) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q71K3HCC044120 for ; Wed, 1 Aug 2012 14:03:27 -0600 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q71K303O009454 for ; Wed, 1 Aug 2012 14:03:00 -0600 From: Anthony Liguori In-Reply-To: <50198508.10303@suse.de> References: <1343049748-11539-1-git-send-email-imammedo@redhat.com> <87zk6elisw.fsf@codemonkey.ws> <50195034.9050201@suse.de> <874nom8o5q.fsf@codemonkey.ws> <50198508.10303@suse.de> Date: Wed, 01 Aug 2012 15:02:52 -0500 Message-ID: <87pq7acrdf.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: peter.maydell@linaro.org, ehabkost@redhat.com, gleb@redhat.com, jan.kiszka@siemens.com, mtosatti@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, blauwirbel@gmail.com, avi@redhat.com, pbonzini@redhat.com, Igor Mammedov Andreas F=C3=A4rber writes: > Am 01.08.2012 20:25, schrieb Anthony Liguori: >> Andreas F=C3=A4rber writes: >>=20 >>> Am 01.08.2012 17:43, schrieb Anthony Liguori: >>>> Igor Mammedov writes: >>>> >>>>> v2: >>>>> ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_c= pu(), >>>>> to keep cpu_init implementation in -softmmu and -user targets the s= ame >>>>> in single place and maintanable. >>>>> >>>>> v3: >>>>> reuse cpu_is_bsp() rather than open code check if apicbase has BSP = bit set >>>>> >>>>> tree for testing: >>>>> https://github.com/imammedo/qemu/tree/x86_reset_v3 >>>>> >>>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets >>>>> >>>>> Igor Mammedov (2): >>>>> target-i386: move cpu halted decision into x86_cpu_reset >>>>> target-i386: move cpu_reset and reset callback to cpu.c >>>> >>>> Applied all. Thanks. >>> >>> So do you intend to refactor all machines accordingly or leave it >>> inconsistent now? >>=20 >> Are you asking me? >>=20 >> No, I have no intention of touching any other machine. We're not going >> to limit cleaning up target-i386 unless every other machine is cleaned >> up too. >>=20 >> Reset logic should live in the CPU. Seems like a no-brainer to me. > > Yes, I'm asking you, since you replied and applied the series without > responding to my review comment on patch 2/2. You probably applied it > locally before reading my comments but then I would still have expected > a reply on how to proceed in light of those comments: No, I saw your comment, although I had already decided to apply it by then. > Before applying this, as I've pointed out to Igor at least once before, > all machines do such reset handling themselves. Patch 2/2 that you > applied makes target-i386 break away from that scheme. (I wonder that > Peter hasn't protested yet...) Devices manage their own reset. CPUs are just another type of device. It's completely logically that CPUs handle their own reset. > Anyway, that being the last patch in this series, I see no value in > doing this on its own for target-i386 only. There's obvious value. You would prefer all targets get refactored too. But that's an unrealistic expectation to place on contributors. > So now we should either > revert that patch and later replace it with one that does a touch-all > change across the boards, or someone needs to volunteer (and you agree, > during the Freeze) to refactor all other machines accordingly, which > will take a while to get Acked-bys from machine maintainers... Or just > defer touching reset callbacks until we have the CPU as a device and > then drop the callbacks instead of moving them. Sorry, but no, this is completely unreasonable. Fighting against improvements because you want more to be improved is counter-productive. No step in the right direction is too small. > Note the point of disagreement here is not "reset logic" - it's great > that the APIC BSP fiddling is gone from PC with patch 1/2 - but the > registration of system-level callbacks in cpu.c in patch 2/2. I thought > we all agreed that we want to make CPU a device and have it reset as a > device? No such callback in cpu.c will be needed then and we thus seem > to be, in absence of follow-ups for 1.2, needlessly moving to-be-dead > code around. Not doing that seems like a no-brainer to me. Devices do one of two things today: 1) register a reset callback 2) implement a reset method that is invoked through it's parent bus Since I don't expect CPUs to exist on a bus, it's not immediately clear to me that (1) isn't going to be what we do for quite some time. Regards, Anthony Liguori > > Regards, > Andreas > > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3= =BCrnberg