[Qemu-devel] [PATCH 0/5] cpu/acpi: convert cpu hot plug to hotplug_handler API

[Qemu-devel] [PATCH 0/5] cpu/acpi: convert cpu hot plug to hotplug_handler API

[Gu Zheng]

Gu Zheng (5):
  acpi/cpu: add cpu hotplug callback function to match hotplug_handler
    API
  acpi:ich9: convert cpu hotplug handle to hotplug_handler API
  acpi:piix4: convert cpu hotplug handle to hotplug_handler API
  pc: add cpu hotplug handler to PC_MACHINE
  cpu/hotplug: remove the left unused cpu hotplug notifier function

 hw/acpi/cpu_hotplug.c         |   18 ++++++++++++------
 hw/acpi/ich9.c                |   15 +++------------
 hw/acpi/piix4.c               |   16 +++-------------
 hw/i386/pc.c                  |   26 +++++++++++++++++++++++++-
 include/hw/acpi/cpu_hotplug.h |    6 ++++--
 include/hw/acpi/ich9.h        |    1 -
 qom/cpu.c                     |    1 -
 7 files changed, 47 insertions(+), 36 deletions(-)

-- 
1.7.7

[Qemu-devel] [PATCH 1/5] acpi/cpu: add cpu hotplug callback function to match hotplug_handler API

[Gu Zheng]

Signed-off-by: Gu Zheng <guz.fnst@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)
+{
+    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);
+    g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
+
+    acpi_update_sci(ar, irq);
+
+    cpu_resume(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);
+
 void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu);
 
 void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
-- 
1.7.7

[Qemu-devel] [PATCH 2/5] acpi:ich9: convert cpu hotplug handle to hotplug_handler API

[Gu Zheng]

Convert notifier based hotplug handle to hotplug_handler API.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 hw/acpi/ich9.c         |   13 ++-----------
 include/hw/acpi/ich9.h |    1 -
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 7b14bbb..89f97d7 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -209,15 +209,6 @@ static void pm_powerdown_req(Notifier *n, void *opaque)
     acpi_pm1_evt_power_down(&pm->acpi_regs);
 }
 
-static void ich9_cpu_added_req(Notifier *n, void *opaque)
-{
-    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, cpu_added_notifier);
-
-    assert(pm != NULL);
-    AcpiCpuHotplug_add(&pm->acpi_regs.gpe, &pm->gpe_cpu, CPU(opaque));
-    acpi_update_sci(&pm->acpi_regs, pm->irq);
-}
-
 void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
                   qemu_irq sci_irq)
 {
@@ -246,8 +237,6 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
 
     AcpiCpuHotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
                         &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
-    pm->cpu_added_notifier.notify = ich9_cpu_added_req;
-    qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
 
     if (pm->acpi_memory_hotplug.is_enabled) {
         acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
@@ -304,6 +293,8 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
         object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         acpi_memory_plug_cb(&pm->acpi_regs, pm->irq, &pm->acpi_memory_hotplug,
                             dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_plug_cb(&pm->acpi_regs, pm->irq, &pm->gpe_cpu, dev);
     } else {
         error_setg(errp, "acpi: device plug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 7e42448..fe975e6 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -47,7 +47,6 @@ typedef struct ICH9LPCPMRegs {
     Notifier powerdown_notifier;
 
     AcpiCpuHotplug gpe_cpu;
-    Notifier cpu_added_notifier;
 
     MemHotplugState acpi_memory_hotplug;
 } ICH9LPCPMRegs;
-- 
1.7.7

[Qemu-devel] [PATCH 3/5] acpi:piix4: convert cpu hotplug handle to hotplug_handler API

[Gu Zheng]

Convert notifier based hotplug handle to hotplug_handler API.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 hw/acpi/piix4.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b72b34e..6209385 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -83,7 +83,6 @@ typedef struct PIIX4PMState {
     uint8_t s4_val;
 
     AcpiCpuHotplug gpe_cpu;
-    Notifier cpu_added_notifier;
 
     MemHotplugState acpi_memory_hotplug;
 } PIIX4PMState;
@@ -348,6 +347,8 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
         acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
                                   errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_plug_cb(&s->ar, s->irq, &s->gpe_cpu, dev);
     } else {
         error_setg(errp, "acpi: device plug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -544,15 +545,6 @@ static const MemoryRegionOps piix4_gpe_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static void piix4_cpu_added_req(Notifier *n, void *opaque)
-{
-    PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
-
-    assert(s != NULL);
-    AcpiCpuHotplug_add(&s->ar.gpe, &s->gpe_cpu, CPU(opaque));
-    acpi_update_sci(&s->ar, s->irq);
-}
-
 static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                                            PCIBus *bus, PIIX4PMState *s)
 {
@@ -565,8 +557,6 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
 
     AcpiCpuHotplug_init(parent, OBJECT(s), &s->gpe_cpu,
                         PIIX4_CPU_HOTPLUG_IO_BASE);
-    s->cpu_added_notifier.notify = piix4_cpu_added_req;
-    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
 
     if (s->acpi_memory_hotplug.is_enabled) {
         acpi_memory_hotplug_init(parent, OBJECT(s), &s->acpi_memory_hotplug);
-- 
1.7.7

[Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE

[Gu Zheng]

Add cpu hotplug handler to PC_MACHINE, which will perform the acpi
cpu hotplug callback via hotplug_handler API.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 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) {
+        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);
     }
 }
 
-- 
1.7.7

[Qemu-devel] [PATCH 5/5] cpu/hotplug: remove the left unused cpu hotplug notifier function

[Gu Zheng]

Remove the left unused cpu hotplug notifier function, and rename
AcpiCpuHotplug_init --> acpi_cpu_hotplug_init
AcpiCpuHotplug_ops --> acpi_cpu_hotplug_ops
to match the coding style.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 hw/acpi/cpu_hotplug.c         |   17 +++--------------
 hw/acpi/ich9.c                |    2 +-
 hw/acpi/piix4.c               |    2 +-
 include/hw/acpi/cpu_hotplug.h |    3 +--
 4 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 92c189b..494d22b 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -26,7 +26,7 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
     /* TODO: implement VCPU removal on guest signal that CPU can be removed */
 }
 
-static const MemoryRegionOps AcpiCpuHotplug_ops = {
+static const MemoryRegionOps acpi_cpu_hotplug_ops = {
     .read = cpu_status_read,
     .write = cpu_status_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
@@ -53,18 +53,7 @@ void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
     cpu_resume(cpu);
 }
 
-void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu)
-{
-    CPUClass *k = CPU_GET_CLASS(cpu);
-    int64_t cpu_id;
-
-    *gpe->sts = *gpe->sts | ACPI_CPU_HOTPLUG_STATUS;
-    cpu_id = k->get_arch_id(CPU(cpu));
-    g_assert((cpu_id / 8) < ACPI_GPE_PROC_LEN);
-    g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
-}
-
-void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
+void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
                          AcpiCpuHotplug *gpe_cpu, uint16_t base)
 {
     CPUState *cpu;
@@ -76,7 +65,7 @@ void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
         g_assert((id / 8) < ACPI_GPE_PROC_LEN);
         gpe_cpu->sts[id / 8] |= (1 << (id % 8));
     }
-    memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
+    memory_region_init_io(&gpe_cpu->io, owner, &acpi_cpu_hotplug_ops,
                           gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
     memory_region_add_subregion(parent, base, &gpe_cpu->io);
 }
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 89f97d7..adf5919 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -235,7 +235,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
     pm->powerdown_notifier.notify = pm_powerdown_req;
     qemu_register_powerdown_notifier(&pm->powerdown_notifier);
 
-    AcpiCpuHotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
+    acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
                         &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
 
     if (pm->acpi_memory_hotplug.is_enabled) {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 6209385..6e91a92 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -555,7 +555,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
     acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
                     s->use_acpi_pci_hotplug);
 
-    AcpiCpuHotplug_init(parent, OBJECT(s), &s->gpe_cpu,
+    acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
                         PIIX4_CPU_HOTPLUG_IO_BASE);
 
     if (s->acpi_memory_hotplug.is_enabled) {
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index d025731..be2f516 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -23,8 +23,7 @@ typedef struct AcpiCpuHotplug {
 void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
                           AcpiCpuHotplug *g, DeviceState *dev);
 
-void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu);
 
-void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
+void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
                          AcpiCpuHotplug *gpe_cpu, uint16_t base);
 #endif
-- 
1.7.7

Re: [Qemu-devel] [PATCH 1/5] acpi/cpu: add cpu hotplug callback function to match hotplug_handler API

[Igor Mammedov]

On Wed,  3 Sep 2014 17:06:13 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> 
> Signed-off-by: Gu Zheng <guz.fnst@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

> +    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.

> +}
> +
>  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 ^^^

> +
>  void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu);
>  
>  void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,

Re: [Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE

[Igor Mammedov]

On Wed,  3 Sep 2014 17:06:16 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
> ---
>  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.

> +        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?

Re: [Qemu-devel] [PATCH 5/5] cpu/hotplug: remove the left unused cpu hotplug notifier function

[Igor Mammedov]

On Wed,  3 Sep 2014 17:06:17 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> Remove the left unused cpu hotplug notifier function, and rename
> AcpiCpuHotplug_init --> acpi_cpu_hotplug_init
> AcpiCpuHotplug_ops --> acpi_cpu_hotplug_ops
> to match the coding style.
split renaming into separate patch please

> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  hw/acpi/cpu_hotplug.c         |   17 +++--------------
>  hw/acpi/ich9.c                |    2 +-
>  hw/acpi/piix4.c               |    2 +-
>  include/hw/acpi/cpu_hotplug.h |    3 +--
>  4 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index 92c189b..494d22b 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -26,7 +26,7 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
>      /* TODO: implement VCPU removal on guest signal that CPU can be removed */
>  }
>  
> -static const MemoryRegionOps AcpiCpuHotplug_ops = {
> +static const MemoryRegionOps acpi_cpu_hotplug_ops = {
>      .read = cpu_status_read,
>      .write = cpu_status_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> @@ -53,18 +53,7 @@ void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
>      cpu_resume(cpu);
>  }
>  
> -void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu)
> -{
> -    CPUClass *k = CPU_GET_CLASS(cpu);
> -    int64_t cpu_id;
> -
> -    *gpe->sts = *gpe->sts | ACPI_CPU_HOTPLUG_STATUS;
> -    cpu_id = k->get_arch_id(CPU(cpu));
> -    g_assert((cpu_id / 8) < ACPI_GPE_PROC_LEN);
> -    g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
> -}

instead of copying contents of this function in 1/5 and deleting it here
could you in 1/5 make a wrapper around this one and drop this function
declaration later in 3/5 with it's last use.

> -
> -void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
> +void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>                           AcpiCpuHotplug *gpe_cpu, uint16_t base)
>  {
>      CPUState *cpu;
> @@ -76,7 +65,7 @@ void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
>          g_assert((id / 8) < ACPI_GPE_PROC_LEN);
>          gpe_cpu->sts[id / 8] |= (1 << (id % 8));
>      }
> -    memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
> +    memory_region_init_io(&gpe_cpu->io, owner, &acpi_cpu_hotplug_ops,
>                            gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
>      memory_region_add_subregion(parent, base, &gpe_cpu->io);
>  }
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 89f97d7..adf5919 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -235,7 +235,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>      pm->powerdown_notifier.notify = pm_powerdown_req;
>      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>  
> -    AcpiCpuHotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
> +    acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
>                          &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>  
>      if (pm->acpi_memory_hotplug.is_enabled) {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 6209385..6e91a92 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -555,7 +555,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>      acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
>                      s->use_acpi_pci_hotplug);
>  
> -    AcpiCpuHotplug_init(parent, OBJECT(s), &s->gpe_cpu,
> +    acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
>                          PIIX4_CPU_HOTPLUG_IO_BASE);
>  
>      if (s->acpi_memory_hotplug.is_enabled) {
> diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
> index d025731..be2f516 100644
> --- a/include/hw/acpi/cpu_hotplug.h
> +++ b/include/hw/acpi/cpu_hotplug.h
> @@ -23,8 +23,7 @@ typedef struct AcpiCpuHotplug {
>  void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
>                            AcpiCpuHotplug *g, DeviceState *dev);
>  
> -void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu);
>  
> -void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
> +void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>                           AcpiCpuHotplug *gpe_cpu, uint16_t base);
>  #endif

Re: [Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE

[Igor Mammedov]

On Wed,  3 Sep 2014 17:06:16 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:


> 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);
^^^ should be dropped as well an hotplug handler should take care about everything this call did.

> -        cpu_resume(cpu);
>      }
>  }
>  

Re: [Qemu-devel] [PATCH 0/5] cpu/acpi: convert cpu hot plug to hotplug_handler API

[Igor Mammedov]

On Wed,  3 Sep 2014 17:06:12 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> 
> Gu Zheng (5):
>   acpi/cpu: add cpu hotplug callback function to match hotplug_handler
>     API
>   acpi:ich9: convert cpu hotplug handle to hotplug_handler API
>   acpi:piix4: convert cpu hotplug handle to hotplug_handler API
>   pc: add cpu hotplug handler to PC_MACHINE
>   cpu/hotplug: remove the left unused cpu hotplug notifier function
> 
>  hw/acpi/cpu_hotplug.c         |   18 ++++++++++++------
>  hw/acpi/ich9.c                |   15 +++------------
>  hw/acpi/piix4.c               |   16 +++-------------
>  hw/i386/pc.c                  |   26 +++++++++++++++++++++++++-
>  include/hw/acpi/cpu_hotplug.h |    6 ++++--
>  include/hw/acpi/ich9.h        |    1 -
>  qom/cpu.c                     |    1 -
>  7 files changed, 47 insertions(+), 36 deletions(-)
> 

Looks like right direction that would allow to drop cpu_added_notifiers
which is not able to pass/handle errors and switch to unified hotplug handler
API which allows to pass errors and would allow to cancel device_add in case of
error.

PS:
Gu Zheng,

Please grep for qemu_register_cpu_added_notifier(), you've missed to convert
rtc usage of CPU notifier.

I'd prefer this to go in before you device_add/add cpu series.

Re: [Qemu-devel] [PATCH 0/5] cpu/acpi: convert cpu hot plug to hotplug_handler API

[Gu Zheng]

Hi Igor,
Thanks very much for your review and suggestion.
On 09/10/2014 10:12 PM, Igor Mammedov wrote:

> On Wed,  3 Sep 2014 17:06:12 +0800
> Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> 
>>
>> Gu Zheng (5):
>>   acpi/cpu: add cpu hotplug callback function to match hotplug_handler
>>     API
>>   acpi:ich9: convert cpu hotplug handle to hotplug_handler API
>>   acpi:piix4: convert cpu hotplug handle to hotplug_handler API
>>   pc: add cpu hotplug handler to PC_MACHINE
>>   cpu/hotplug: remove the left unused cpu hotplug notifier function
>>
>>  hw/acpi/cpu_hotplug.c         |   18 ++++++++++++------
>>  hw/acpi/ich9.c                |   15 +++------------
>>  hw/acpi/piix4.c               |   16 +++-------------
>>  hw/i386/pc.c                  |   26 +++++++++++++++++++++++++-
>>  include/hw/acpi/cpu_hotplug.h |    6 ++++--
>>  include/hw/acpi/ich9.h        |    1 -
>>  qom/cpu.c                     |    1 -
>>  7 files changed, 47 insertions(+), 36 deletions(-)
>>
> 
> Looks like right direction that would allow to drop cpu_added_notifiers
> which is not able to pass/handle errors and switch to unified hotplug handler
> API which allows to pass errors and would allow to cancel device_add in case of
> error.

It seems the key value we can gain from above change, I'll follow your suggestion. 

> 
> PS:
> Gu Zheng,
> 
> Please grep for qemu_register_cpu_added_notifier(), you've missed to convert
> rtc usage of CPU notifier.

Thanks for your reminder.

> 
> I'd prefer this to go in before you device_add/add cpu series.

OK, I'll work on this series first.

Best regards,
Gu

> .
> 

Re: [Qemu-devel] [PATCH 1/5] acpi/cpu: add cpu hotplug callback function to match hotplug_handler API

[Gu Zheng]

Hi Igor,
On 09/10/2014 09:28 PM, Igor Mammedov wrote:

> On Wed,  3 Sep 2014 17:06:13 +0800
> Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> 
>>
>> Signed-off-by: Gu Zheng <guz.fnst@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.

> 
>> +}
>> +
>>  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,
> 
> 
> .
> 

Re: [Qemu-devel] [PATCH 1/5] acpi/cpu: add cpu hotplug callback function to match hotplug_handler API

[Igor Mammedov]

On Thu, 11 Sep 2014 11:04:10 +0800
Gu Zheng <guz.fnst@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.fnst@cn.fujitsu.com> wrote:
> > 
> >>
> >> Signed-off-by: Gu Zheng <guz.fnst@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,
> > 
> > 
> > .
> > 
> 
> 

Re: [Qemu-devel] [PATCH 1/5] acpi/cpu: add cpu hotplug callback function to match hotplug_handler API

[Gu Zheng]

Hi Igor,
On 09/11/2014 06:12 PM, Igor Mammedov wrote:

> On Thu, 11 Sep 2014 11:04:10 +0800
> Gu Zheng <guz.fnst@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.fnst@cn.fujitsu.com> wrote:
>>>
>>>>
>>>> Signed-off-by: Gu Zheng <guz.fnst@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).

Seems I had a misreading on this, thanks for your explanation.

Best regards,
Gu

> 
>>
>>>
>>>> +}
>>>> +
>>>>  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,
>>>
>>>
>>> .
>>>
>>
>>
> 
> .
> 

Re: [Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE

[Gu Zheng]

Hi Igor,
On 09/10/2014 09:55 PM, Igor Mammedov wrote:

> On Wed,  3 Sep 2014 17:06:16 +0800
> Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
>> ---
>>  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

> 
> .
> 

Re: [Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE

[Igor Mammedov]

On Fri, 12 Sep 2014 11:02:09 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> Hi Igor,
> On 09/10/2014 09:55 PM, Igor Mammedov wrote:
> 
> > On Wed,  3 Sep 2014 17:06:16 +0800
> > Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
> >> ---
> >>  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.

then check for !pcms->acpi_dev and exit if it not set (i.e. as pc_dimm_plug() does)
since there isn't acpi hardware to update for startup CPUs either.

may be something along this lines:

if (!pcms->acpi_dev) {
  if (dev->hotplugged) error_setg(&local_err, "CPU hotplug is not supported without ACPI");
  goto out;
}

> 
> > 
> >> +        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.
You still need to set hotplug handler for CPU device or ICCBus
grep for qbus_set_hotplug_handler()

> 
>  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
> 
> > 
> > .
> > 
> 
> 

Re: [Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE

[Gu Zheng]

Hi Igor,

On 09/12/2014 10:28 PM, Igor Mammedov wrote:

> On Fri, 12 Sep 2014 11:02:09 +0800
> Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> 
>> Hi Igor,
>> On 09/10/2014 09:55 PM, Igor Mammedov wrote:
>>
>>> On Wed,  3 Sep 2014 17:06:16 +0800
>>> Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
>>>> ---
>>>>  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.
> 
> then check for !pcms->acpi_dev and exit if it not set (i.e. as pc_dimm_plug() does)
> since there isn't acpi hardware to update for startup CPUs either.
> 
> may be something along this lines:
> 
> if (!pcms->acpi_dev) {
>   if (dev->hotplugged) error_setg(&local_err, "CPU hotplug is not supported without ACPI");
>   goto out;
> }

This is more neat, thanks.

> 
>>
>>>
>>>> +        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.
> You still need to set hotplug handler for CPU device or ICCBus
> grep for qbus_set_hotplug_handler()

Current implementation is the same as bus-less device's (e.g. memory), via the
machine's get_hotplug_handler method to discover a hotplug handler controller,
and it works fine.
Shall we still need to set hotplug handler ICCBus, which seems duplicated?
Or switch to set hotplug handler to ICCBus, and drop current implementation?

Thanks,
Gu

> 
>>
>>  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
>>
>>>
>>> .
>>>
>>
>>
> 
> .
> 

Re: [Qemu-devel] [PATCH 4/5] pc: add cpu hotplug handler to PC_MACHINE

[Igor Mammedov]

On Mon, 15 Sep 2014 11:57:56 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> Hi Igor,
> 
> On 09/12/2014 10:28 PM, Igor Mammedov wrote:
> 
> > On Fri, 12 Sep 2014 11:02:09 +0800
> > Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> > 
> >> Hi Igor,
> >> On 09/10/2014 09:55 PM, Igor Mammedov wrote:
> >>
> >>> On Wed,  3 Sep 2014 17:06:16 +0800
> >>> Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
> >>>> ---
> >>>>  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.
> > 
> > then check for !pcms->acpi_dev and exit if it not set (i.e. as pc_dimm_plug() does)
> > since there isn't acpi hardware to update for startup CPUs either.
> > 
> > may be something along this lines:
> > 
> > if (!pcms->acpi_dev) {
> >   if (dev->hotplugged) error_setg(&local_err, "CPU hotplug is not supported without ACPI");
> >   goto out;
> > }
> 
> This is more neat, thanks.
> 
> > 
> >>
> >>>
> >>>> +        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.
> > You still need to set hotplug handler for CPU device or ICCBus
> > grep for qbus_set_hotplug_handler()
> 
> Current implementation is the same as bus-less device's (e.g. memory), via the
> machine's get_hotplug_handler method to discover a hotplug handler controller,
> and it works fine.
I'm sorry, I've forgot about default machine hotplug handler.

> Shall we still need to set hotplug handler ICCBus, which seems duplicated?
> Or switch to set hotplug handler to ICCBus, and drop current implementation?
Taking in account that we should access RTC device from handler,
machine's hotplug handler serves purpose just fine.

> 
> Thanks,
> Gu
> 
> > 
> >>
> >>  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
> >>
> >>>
> >>> .
> >>>
> >>
> >>
> > 
> > .
> > 
> 
>