From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49134) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYXNW-0006o7-0Y for qemu-devel@nongnu.org; Mon, 29 Sep 2014 05:36:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYXNP-0000We-6h for qemu-devel@nongnu.org; Mon, 29 Sep 2014 05:36:21 -0400 Received: from [59.151.112.132] (port=63921 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYXNO-0000Va-SE for qemu-devel@nongnu.org; Mon, 29 Sep 2014 05:36:15 -0400 Message-ID: <542924C0.2030202@cn.fujitsu.com> Date: Mon, 29 Sep 2014 17:22:08 +0800 From: Gu Zheng MIME-Version: 1.0 References: <1410917043-16334-1-git-send-email-guz.fnst@cn.fujitsu.com> <1410917043-16334-8-git-send-email-guz.fnst@cn.fujitsu.com> <20140926153717.1a87a0ee@nial.usersys.redhat.com> In-Reply-To: <20140926153717.1a87a0ee@nial.usersys.redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V3 7/7] acpi/cpu-hotplug: introduce help function to keep bit setting in one place 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/26/2014 09:37 PM, Igor Mammedov wrote: > On Wed, 17 Sep 2014 09:24:03 +0800 > Gu Zheng wrote: > >> Introduce help function acpi_set_local_sts() to simplify acpi_cpu_plug_cb >> and acpi_cpu_hotplug_init, so that we can keep bit setting in one place. >> >> Signed-off-by: Gu Zheng >> --- >> hw/acpi/cpu_hotplug.c | 22 +++++++++++++++------- >> 1 files changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c >> index 7629686..d42c961 100644 >> --- a/hw/acpi/cpu_hotplug.c >> +++ b/hw/acpi/cpu_hotplug.c >> @@ -36,8 +36,8 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = { >> }, >> }; >> >> -void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq, >> - AcpiCpuHotplug *g, CPUState *cpu, Error **errp) >> +static void acpi_set_local_sts(AcpiCpuHotplug *g, CPUState *cpu, >> + Error **errp) >> { >> CPUClass *k = CPU_GET_CLASS(cpu); >> int64_t cpu_id; >> @@ -48,9 +48,18 @@ void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq, >> return; >> } >> >> - ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS; >> g->sts[cpu_id / 8] |= (1 << (cpu_id % 8)); >> +} >> >> +void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq, >> + AcpiCpuHotplug *g, CPUState *cpu, Error **errp) >> +{ >> + acpi_set_local_sts(g, cpu, errp); >> + if (*errp != NULL) { >> + return; >> + } >> + > >> + ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS; > ^^^ shouldn't be set for coldplugged CPUs along with IRQ below > >> acpi_update_sci(ar, irq); >> } >> >> @@ -60,11 +69,10 @@ void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, >> CPUState *cpu; >> >> CPU_FOREACH(cpu) { >> - CPUClass *cc = CPU_GET_CLASS(cpu); >> - int64_t id = cc->get_arch_id(cpu); >> + Error *local_err = NULL; >> >> - g_assert((id / 8) < ACPI_GPE_PROC_LEN); >> - gpe_cpu->sts[id / 8] |= (1 << (id % 8)); >> + acpi_set_local_sts(gpe_cpu, cpu, &local_err); >> + g_assert(local_err == NULL); > Wouldn't acpi_cpu_plug_cb() do everything this FOREACH does? > I've suggest to drop CPU_FOREACH() altogether. I got your meaning: Make acpi_cpu_plug_cb serve all CPUs (both startup and hotpluged) , and only triggers sci irq for hotpluged ones, so that we can drop the duplicated operations in acpi_cpu_hotplug_init. But one problem is that pcms->acpi_dev was registered after startup CPUs set realized, so acpi_cpu_plug_cb can not serve startup CPUs. I tried to switch the order, but it failed, because there are some internal dependences. Now, I'm trying to split the startup CPUs init into two steps (init and realize), and register pcms->acpi_dev between the two steps. What's your opinion? Thanks, Gu > >> } >> memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops, >> gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN); > > . >