From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37445) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSHKy-0001BJ-Sk for qemu-devel@nongnu.org; Thu, 11 Sep 2014 23:15:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XSHKu-0006eS-66 for qemu-devel@nongnu.org; Thu, 11 Sep 2014 23:15:52 -0400 Received: from [59.151.112.132] (port=17152 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSHKt-0006e5-R5 for qemu-devel@nongnu.org; Thu, 11 Sep 2014 23:15:48 -0400 Message-ID: <54126231.2080709@cn.fujitsu.com> Date: Fri, 12 Sep 2014 11:02:09 +0800 From: Gu Zheng MIME-Version: 1.0 References: <1409735177-17232-1-git-send-email-guz.fnst@cn.fujitsu.com> <1409735177-17232-5-git-send-email-guz.fnst@cn.fujitsu.com> <20140910155508.6ae4f329@nial.usersys.redhat.com> In-Reply-To: <20140910155508.6ae4f329@nial.usersys.redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: chen.fan.fnst@cn.fujitsu.com, isimatu.yasuaki@jp.fujitsu.com, tangchen@cn.fujitsu.com, qemu-devel@nongnu.org, afaerber@suse.de Hi Igor, On 09/10/2014 09:55 PM, Igor Mammedov wrote: > On Wed, 3 Sep 2014 17:06:16 +0800 > Gu Zheng wrote: > >> Add cpu hotplug handler to PC_MACHINE, which will perform the acpi >> cpu hotplug callback via hotplug_handler API. >> >> Signed-off-by: Gu Zheng >> --- >> hw/i386/pc.c | 26 +++++++++++++++++++++++++- >> qom/cpu.c | 1 - >> 2 files changed, 25 insertions(+), 2 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 8fa8d2f..c2956f9 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1607,11 +1607,34 @@ out: >> error_propagate(errp, local_err); >> } >> >> +static void pc_cpu_plug(HotplugHandler *hotplug_dev, >> + DeviceState *dev, Error **errp) >> +{ >> + HotplugHandlerClass *hhc; >> + Error *local_err = NULL; >> + PCMachineState *pcms = PC_MACHINE(hotplug_dev); >> + >> + if (dev->hotplugged) { > for startup CPUs we set gpe_cpu status bits in AcpiCpuHotplug_init() > and for hotpluged in AcpiCpuHotplug_add() which is duplicating > essentially the same code. > > Could you drop above check and make AcpiCpuHotplug_init() take care > about startup CPUs as well, keeping bit setting in one place. Yeah, with the above change, code will be more neat, but I think it is not a serious problem.:) And pcms->acpi_dev is set when PC hardware initialisation (e.g. pc_init1), for start up cpus, the plug handler is not valid, so the check is needed here to avoid trigger error by start cpu. > >> + if (!pcms->acpi_dev) { >> + error_setg(&local_err, >> + "cpu hotplug is not enabled: missing acpi device"); >> + goto out; >> + } >> + >> + hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); >> + hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); >> +out: >> + error_propagate(errp, local_err); >> + } >> +} >> + >> static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp) >> { >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { >> pc_dimm_plug(hotplug_dev, dev, errp); >> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { >> + pc_cpu_plug(hotplug_dev, dev, errp); >> } >> } >> >> @@ -1620,7 +1643,8 @@ static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, >> { >> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine); >> >> - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { >> + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) >> + || object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { >> return HOTPLUG_HANDLER(machine); >> } >> >> diff --git a/qom/cpu.c b/qom/cpu.c >> index b32dd0a..af8e83f 100644 >> --- a/qom/cpu.c >> +++ b/qom/cpu.c >> @@ -304,7 +304,6 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp) >> if (dev->hotplugged) { >> cpu_synchronize_post_init(cpu); >> notifier_list_notify(&cpu_added_notifiers, dev); >> - cpu_resume(cpu); >> } >> } >> > > I don't see what sets PCMachine as hotplug_handler for CPU, > maybe series is missing a patch? Previous memory hotplug has built the frame work, here just adding the case to handle CPU. static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { pc_dimm_plug(hotplug_dev, dev, errp); + } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { + pc_cpu_plug(hotplug_dev, dev, errp); } Thanks, Gu > > . >