From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54271) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aXxVY-00006k-VJ for qemu-devel@nongnu.org; Mon, 22 Feb 2016 15:55:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aXxVV-0001Bn-Nf for qemu-devel@nongnu.org; Mon, 22 Feb 2016 15:55:04 -0500 Received: from e19.ny.us.ibm.com ([129.33.205.209]:46463) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aXxVV-0001BW-Js for qemu-devel@nongnu.org; Mon, 22 Feb 2016 15:55:01 -0500 Received: from localhost by e19.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 22 Feb 2016 15:55:01 -0500 Received: from b01cxnp22035.gho.pok.ibm.com (b01cxnp22035.gho.pok.ibm.com [9.57.198.25]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id D406C38C8026 for ; Mon, 22 Feb 2016 15:54:58 -0500 (EST) Received: from d01av05.pok.ibm.com (d01av05.pok.ibm.com [9.56.224.195]) by b01cxnp22035.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u1MKswTg29687968 for ; Mon, 22 Feb 2016 20:54:58 GMT Received: from d01av05.pok.ibm.com (localhost [127.0.0.1]) by d01av05.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u1MKpAhZ021079 for ; Mon, 22 Feb 2016 15:51:10 -0500 References: <1456160797-832-1-git-send-email-mjrosato@linux.vnet.ibm.com> <1456160797-832-8-git-send-email-mjrosato@linux.vnet.ibm.com> <56CB43E2.1060207@suse.de> From: Matthew Rosato Message-ID: <56CB759D.70901@linux.vnet.ibm.com> Date: Mon, 22 Feb 2016 15:54:53 -0500 MIME-Version: 1.0 In-Reply-To: <56CB43E2.1060207@suse.de> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Andreas_F=c3=a4rber?= , qemu-devel@nongnu.org Cc: borntraeger@de.ibm.com, agraf@suse.de, dahi@linux.vnet.ibm.com, pbonzini@redhat.com, bharata@linux.vnet.ibm.com, cornelia.huck@de.ibm.com, imammedo@redhat.com, rth@twiddle.net On 02/22/2016 12:22 PM, Andreas Färber wrote: > Am 22.02.2016 um 18:06 schrieb Matthew Rosato: >> Implement cpu hotplug routine and add the machine hook. >> >> Signed-off-by: Matthew Rosato >> --- >> hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++ >> target-s390x/cpu.c | 7 +++++++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 3090e76..ea007e8 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -22,6 +22,7 @@ >> #include "s390-pci-bus.h" >> #include "hw/s390x/storage-keys.h" >> #include "hw/compat.h" >> +#include "qom/cpu.h" >> >> #define TYPE_S390_CCW_MACHINE "s390-ccw-machine" >> >> @@ -185,6 +186,37 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, >> return NULL; >> } >> >> +static void s390_hot_add_cpu(const int64_t id, Error **errp) >> +{ >> + MachineState *machine = MACHINE(qdev_get_machine()); >> + >> + if (id < 0) { >> + error_setg(errp, "Invalid CPU id: %" PRIi64, id); >> + return; >> + } >> + >> + if (cpu_exists(id)) { >> + error_setg(errp, "Unable to add CPU: %" PRIi64 >> + ", it already exists", id); >> + return; >> + } >> + >> + if (id >= max_cpus) { >> + error_setg(errp, "Unable to add CPU: %" PRIi64 >> + ", max allowed: %d", id, max_cpus - 1); >> + return; >> + } >> + >> + if (id != (last_cpu->cpu_index + 1)) { > > This assumption about the order of the CPU list looks dangerous to me. > Aren't there global int fields used by x86 already for the number of > CPUs that you could use instead? That will allow you to drop the ugly > preceding renaming patch. Please avoid messing with the CPU list > directly from target code. > Not that I can find. We were keeping track of it with our own int for s390x, I just switched to this approach per review suggestion -- Sounds like you'd rather I bring back the int and inspect that, that's fine by me. >> + error_setg(errp, "Unable to add CPU: %" PRIi64 >> + ", The next available id is %d", id, >> + (last_cpu->cpu_index + 1)); >> + return; >> + } >> + >> + cpu_s390x_init(machine->cpu_model); > > No proper error handling - that's why blindly reusing these old helpers > is a bad idea. > Good point -- Let me re-work this into something more like f(model, id, local_err). Matt