From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54034) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwfLX-0002W1-EO for qemu-devel@nongnu.org; Wed, 01 Aug 2012 16:16:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwfLV-0006IA-NE for qemu-devel@nongnu.org; Wed, 01 Aug 2012 16:16:43 -0400 Received: from cantor2.suse.de ([195.135.220.15]:47083 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwfLV-0006I5-D6 for qemu-devel@nongnu.org; Wed, 01 Aug 2012 16:16:41 -0400 Message-ID: <50198EA3.9070109@suse.de> Date: Wed, 01 Aug 2012 22:16:35 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 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> <87pq7acrdf.fsf@codemonkey.ws> In-Reply-To: <87pq7acrdf.fsf@codemonkey.ws> 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: Anthony Liguori 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 Am 01.08.2012 22:02, schrieb Anthony Liguori: > Andreas F=C3=A4rber writes: >=20 >> Am 01.08.2012 20:25, schrieb Anthony Liguori: >>> Andreas F=C3=A4rber writes: >>> >>>> 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_ne= w_cpu(), >>>>>> to keep cpu_init implementation in -softmmu and -user targets th= e same >>>>>> in single place and maintanable. >>>>>> >>>>>> v3: >>>>>> reuse cpu_is_bsp() rather than open code check if apicbase has B= SP 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 target= s >>>>>> >>>>>> 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? >>> >>> Are you asking me? >>> >>> No, I have no intention of touching any other machine. We're not goi= ng >>> to limit cleaning up target-i386 unless every other machine is cleane= d >>> up too. >>> >>> 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 expecte= d >> a reply on how to proceed in light of those comments: >=20 > No, I saw your comment, although I had already decided to apply it by > then. >=20 >> 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...) >=20 > Devices manage their own reset. CPUs are just another type of device. > It's completely logically that CPUs handle their own reset. >=20 >> Anyway, that being the last patch in this series, I see no value in >> doing this on its own for target-i386 only. >=20 > There's obvious value. You would prefer all targets get refactored > too. But that's an unrealistic expectation to place on contributors. >=20 >> 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. >=20 > 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. >=20 >> 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 though= t >> 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. >=20 > Devices do one of two things today: >=20 > 1) register a reset callback >=20 > 2) implement a reset method that is invoked through it's parent bus >=20 > 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. Err, I thought devices implement a function assigned to a DeviceClass::reset, no? That would be (2) on your list and we've been working on ripping out (1) for devices, on sPAPR for instance. (2) is what we already have with CPUClass::reset. The only remaining issue is that the CPUClass::reset callback is not automatically called on machine/bus reset yet. And what I was saying is that moving the code is NOT an improvement. It is NO functional change and it is NOT a prerequisite for any change on the list today. So it is not needed for the to be released 1.2. A very low hanging fruit for 1.2 would be to register a SINGLE central reset callback that iterates through the globally available CPU list and calls ->reset on each! Then we can drop the reset callbacks in most machines rather than moving old code around. Regards, Andreas >=20 > Regards, >=20 > Anthony Liguori >=20 >> >> 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 >=20 --=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=BC= rnberg