From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54016) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNRx2-00089Y-97 for qemu-devel@nongnu.org; Wed, 03 Apr 2013 13:58:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UNRx1-0000WZ-5u for qemu-devel@nongnu.org; Wed, 03 Apr 2013 13:58:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27409) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNRx0-0000WK-UC for qemu-devel@nongnu.org; Wed, 03 Apr 2013 13:58:23 -0400 Date: Wed, 3 Apr 2013 19:58:00 +0200 From: Igor Mammedov Message-ID: <20130403195800.0ba38817@thinkpad.mammed.net> In-Reply-To: <5152D5A5.4030405@redhat.com> References: <1363876125-8264-1-git-send-email-imammedo@redhat.com> <1363876125-8264-13-git-send-email-imammedo@redhat.com> <5152D5A5.4030405@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, ehabkost@redhat.com, mst@redhat.com, jan.kiszka@siemens.com, stefano.stabellini@eu.citrix.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, aderumier@odiso.com, armbru@redhat.com, blauwirbel@gmail.com, quintela@redhat.com, alex.williamson@redhat.com, kraxel@redhat.com, anthony.perard@citrix.com, yang.z.zhang@intel.com, lcapitulino@redhat.com, afaerber@suse.de, rth@twiddle.net On Wed, 27 Mar 2013 12:19:01 +0100 Paolo Bonzini wrote: > Il 21/03/2013 15:28, Igor Mammedov ha scritto: > > ... via do_cpu_hot_add() hook called by cpu_set QMP command, > > for x86 target. > > > > * add extra check that APIC ID is in allowed range > > * return error if CPU with requested APIC ID exists before creating > > a new instance. (CPU removal is broken now, will be fixed with CPU unplug) > > * call CPU add notifier as the last step of x86_cpu_realizefn() to > > update rtc_cmos and trigger CPU hot-plug ACPI GPE to notify guest. > > Doing it from x86_cpu_realizefn() will allow to do the same when > > it would be possible to add CPU using device_add. > > > > Signed-off-by: Igor Mammedov > > --- > > hw/i386/pc.c | 22 ++++++++++++++++++++++ > > target-i386/cpu.c | 1 + > > 2 files changed, 23 insertions(+), 0 deletions(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 7481f73..e3ba9ee 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -867,6 +867,19 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp) > > { > > X86CPU *cpu; > > > > + if (apic_id >= pc_apic_id_limit(max_cpus)) { > > + error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64 > > + ", max allowed ID: 0x%x", apic_id, > > + pc_apic_id_limit(max_cpus) - 1); > > + return; > > + } > > This seems the wrong place to do this check. It should be done in > do_cpu_hot_add, simply comparing against max_cpus. Here, instead, you > should _assert_ that the APIC ID is in range. I'll move it there, but I'd leave pc_apic_id_limit(max_cpus) since id it compares with is APIC ID. > > > + if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) { > > Similarly, can this be done in qmp_cpu_set? And should it really be an I've tried to abstract qmp part from target/implementation details. We could introduce global func like, and then use it if from QMP subsystem: bool is_cpu_exist(int64_t id) { ... iterate over CPUs ... if (cpu->get_firmware_id() == id) return true; ... } Andreas, Is it acceptable if I add it to qom/cpu.h, qom/cpu.c ? > error? Onlining an already-online CPU is fine. CPU is not just onlined though, it's created and it couldn't be right to create the same CPU. Hence a error here, so user would know that it tries to add the same device. > > Again, here you could assert that the CPU is not a duplicate, instead. sure > > > + error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64 > > + ", it's already exists", apic_id); > > + return; > > + } > > cpu = cpu_x86_create(cpu_model, errp); > > if (!cpu) { > > return; > > @@ -882,6 +895,8 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp) > > } > > } > > > > +static const char *saved_cpu_model; > > + > > void pc_cpus_init(const char *cpu_model) > > Instead of using this global, see the approach in the previous patch. > > > { > > int i; > > @@ -895,6 +910,8 @@ void pc_cpus_init(const char *cpu_model) > > cpu_model = "qemu32"; > > #endif > > } > > + saved_cpu_model = cpu_model; > > + > > > for (i = 0; i < smp_cpus; i++) { > > pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error); > > @@ -906,6 +923,11 @@ void pc_cpus_init(const char *cpu_model) > > } > > } > > > > +void do_cpu_hot_add(const int64_t id, Error **errp) > > +{ > > + pc_new_cpu(saved_cpu_model, id, errp); > > +} > > + > > Missing x86_cpu_apic_id_from_index(id)? There was(is?) opposition to using cpu_index to identify x86 CPU. So, it is expected from management to provide APIC ID instead of cpu_index. It could be useful to make hotplug to a specific NUMA node/cpu to work in future. Though interface of possible APIC IDs discovery is not part of this series. > > > void pc_acpi_init(const char *default_dsdt) > > { > > char *filename = NULL, *arg = NULL; > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index ae46f81..d127141 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -2271,6 +2271,7 @@ out: > > if (dev->hotplugged) { > > cpu_synchronize_post_init(env); > > resume_vcpu(CPU(cpu)); > > + qemu_system_cpu_hotplug_request(env->cpuid_apic_id); > > As mentioned earlier, this notifier should be invoked at the CPU level, > not X86CPU. done > Paolo > > > } > > } > > > > > > -- Regards, Igor