From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51231) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLWGy-0007Y8-SQ for qemu-devel@nongnu.org; Mon, 16 Sep 2013 06:43:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VLWGs-0004zE-0V for qemu-devel@nongnu.org; Mon, 16 Sep 2013 06:43:16 -0400 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:37831) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLWGr-0004v9-OO for qemu-devel@nongnu.org; Mon, 16 Sep 2013 06:43:09 -0400 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 16 Sep 2013 11:32:32 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 6B1DF2190056 for ; Mon, 16 Sep 2013 11:43:04 +0100 (BST) Received: from d06av10.portsmouth.uk.ibm.com (d06av10.portsmouth.uk.ibm.com [9.149.37.251]) by b06cxnps3075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r8GAgqMD41222260 for ; Mon, 16 Sep 2013 10:42:52 GMT Received: from d06av10.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av10.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r8GAh3hu032102 for ; Mon, 16 Sep 2013 04:43:04 -0600 Date: Mon, 16 Sep 2013 12:43:02 +0200 From: Michael Mueller Message-ID: <20130916124302.3b6cf8bb@bee> In-Reply-To: <523328E5.4000305@linux.vnet.ibm.com> References: <1375366359-11553-1-git-send-email-jjherne@us.ibm.com> <0C872A4D-5B78-4F76-A254-41FDC3147896@suse.de> <522881AC.6030404@suse.de> <52288FC8.7090200@suse.de> <523328E5.4000305@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 0/8] [PATCH RFC v3] s390 cpu hotplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: jjherne@linux.vnet.ibm.com Cc: Alexander Graf , ehabkost@redhat.com, qemu-devel@nongnu.org, "Jason J. Herne" , borntraeger@de.ibm.com, jfrei@linux.vnet.ibm.com, imammedo@redhat.com, Andreas =?UTF-8?B?RsOkcmJlcg==?= On Fri, 13 Sep 2013 11:01:57 -0400 "Jason J. Herne" wrote: > On 09/05/2013 10:06 AM, Andreas F=C3=A4rber wrote: > > Am 05.09.2013 15:10, schrieb Alexander Graf: > >> On 05.09.2013, at 15:05, Andreas F=C3=A4rber wrote: > >>> Am 05.09.2013 14:54, schrieb Alexander Graf: > >>>> Very simple and clean patch set. I don't think it deserves the RFC t= ag. > >>> > >>> Negative, see my review. If you want to fix up and queue patches 1-2 > >>> that's fine with me, but the others need a respin. No major blocker > >>> though, just some more footwork mostly related to QOM and Jason's > >>> shifted focus on cpu-add rather than device_add. > >> > >> Yeah, that's what I'm referring to. I've seen a lot worse patch sets a= t v8 than this RFC :). > >> > >> I don't think we should apply it as is, and I'm very happy to see your= review and comment on > >> the modeling bits :). But I try to never apply or cherry pick RFC patc= hes - and this set > >> looks like he sent it with the intent of getting it merged. > > > > Agreed, we can continue with "PATCH v4". I was more upset about the > > "very simple and clean" bit after I commented on a number of unclean > > things to improve - mostly about doing things in different places. > > > > If you could find some time to review my two model string patches then I > > could supply Jason with a branch or even a pull to base on: > > > > http://patchwork.ozlabs.org/patch/272511/ > > http://patchwork.ozlabs.org/patch/272509/ > > > > I would also volunteer to provide a base patch for the link<> issue if > > there is agreement. Apart from the QOM API question this depends on the > > contradictory modelling of whether we allow CPU addresses 0..max_cpus as > > seen in this series or 0..somemax with <=3D max_cpus non-NULL as discus= sed > > on #zkvm. >=20 > According to http://wiki.qemu.org/Features/CPUHotplug: >=20 > "adding CPUs should be done in successive order from lower to higher IDs= =20 > in [0..max-cpus) range. > It's possible to add arbitrary CPUs in random order, however that would=20 > cause migration to fail on its target side." >=20 > Considering that, in a virtual environment, it rarely (if ever) makes=20 > sense to define out of order cpu ids maybe we should keep the patch as=20 > is and only allow consecutive cpu ids to be used. >=20 > By extension, hot-unplug would require that the highest id be unplugged.= =20 > This is probably not acceptable in any type of "mixed cpu" environment=20 > because the greatest id may not be the cpu type you want to remove. I'm= =20 > not sure if S390 will implement mixed cpu types. As zArch implements only one CPU type in its CEC we also will allow only vi= rtual CPUs of the same type in one KVM. >=20 > > (child properties would allow to model the latter sparse > > address space very well, but an object can only have one parent in the > > hot-add case. We could of course add cpu[n] link properties as > > CPUs get added, but that doesn't strike me as very clean. My underlying > > thought is to offload the error handling to QOM so that we don't start > > hardcoding s/smp_cpus/max_cpus/g (or some max_cpu_address) all around > > ipi_states.) > > >=20 > I'm not sure I understand. What is meant by: "an object can only have=20 > one parent in the hot-add case." >=20 > What is the difference between "child" and "cpu[n]=20 > link"? And why do you feel the link case would be unclean? >=20 > > Btw an unanswered question: ipi_states is just pointers to CPUs > > currently, no further state. So what's "ipi" in the name? Will that > > array need to carry state beyond S390CPU someday? > > >=20 > Quoting Jens Freimann: > " > The ipi_states array holds all our S390CPU states. The position of a cpu > within this array equals its "cpu address". See section "CPU address=20 > identification" > in the Principles of Operation. This cpu address is used for > cpu signalling (inter-processor interrupts->ipi) via the sigp instruction. > The cpu address does not contain information about which book this cpu is > plugged into, nor does it hold any other topology information. >=20 > When we intercept a sigp instruction in qemu the cpu address is passed > to us in a field of the SIE control block. We then use the cpu address > as an index to the ipi_states field, get hold of the S390CPU and then > for example do an vcpu ioctl on the chosen cpu. > " >=20 > Based on this explanation I do not think this array will ever be=20 > anything more than what it is today. >=20