On Thu, 11 Sep 2014 11:04:10 +0800 Gu Zheng <guz.f...@cn.fujitsu.com> wrote:
> Hi Igor, > On 09/10/2014 09:28 PM, Igor Mammedov wrote: > > > On Wed, 3 Sep 2014 17:06:13 +0800 > > Gu Zheng <guz.f...@cn.fujitsu.com> wrote: > > > >> > >> Signed-off-by: Gu Zheng <guz.f...@cn.fujitsu.com> > >> --- > >> hw/acpi/cpu_hotplug.c | 17 +++++++++++++++++ > >> include/hw/acpi/cpu_hotplug.h | 3 +++ > >> 2 files changed, 20 insertions(+), 0 deletions(-) > >> > >> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c > >> index 2ad83a0..92c189b 100644 > >> --- a/hw/acpi/cpu_hotplug.c > >> +++ b/hw/acpi/cpu_hotplug.c > >> @@ -36,6 +36,23 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = { > >> }, > >> }; > >> > >> +void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq, > >> + AcpiCpuHotplug *g, DeviceState *dev) > > wrong indentation ^^^ > > > > it wouldn't hurt to add errp argument here ... > > > >> +{ > >> + CPUState *cpu = CPU(dev); > >> + CPUClass *k = CPU_GET_CLASS(cpu); > >> + int64_t cpu_id; > >> + > >> + ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS; > >> + cpu_id = k->get_arch_id(cpu); > >> + g_assert((cpu_id / 8) < ACPI_GPE_PROC_LEN); > > ... and return error from here instead of aborting > > Got it. > > > > >> + g->sts[cpu_id / 8] |= (1 << (cpu_id % 8)); > >> + > >> + acpi_update_sci(ar, irq); > >> + > >> + cpu_resume(cpu); > > Why are you adding cpu_resume() here? > > check cpu_common_realizefn() which already does it. > > Because hot added callback is called after cpu_common_realizefn(), so I moved > cpu_resume(cpu) from cpu_common_realizefn() here to ensure the guest has > already > hot added cpu before we resume it. cpu.realize() should create a fully working CPU and moving CPU internals to ACPI is not correct. CPU hot-add shouyld not depend on whether guest OS supports it or not. More over there is no need to resume CPU when guest OS hot-added it, cpu.realize() creates secondary CPU in RESET state, so it does nothing and waits for INIT/SIPI sequence (i.e. when guest OS decides to online CPU). > > > > >> +} > >> + > >> void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu) > >> { > >> CPUClass *k = CPU_GET_CLASS(cpu); > >> diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h > >> index 9e5d30c..d025731 100644 > >> --- a/include/hw/acpi/cpu_hotplug.h > >> +++ b/include/hw/acpi/cpu_hotplug.h > >> @@ -20,6 +20,9 @@ typedef struct AcpiCpuHotplug { > >> uint8_t sts[ACPI_GPE_PROC_LEN]; > >> } AcpiCpuHotplug; > >> > >> +void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq, > >> + AcpiCpuHotplug *g, DeviceState *dev); > > wrong indentation ^^^ > > OK, will fix this too. > > Thanks, > Gu > > > > >> + > >> void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu); > >> > >> void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner, > > > > > > . > > > >