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

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

[Gu Zheng]

Previously we use cpu_added_notifiers to register cpu hotplug notifier callback
which is not able to pass/handle errors, so we switch it to unified hotplug
handler API which allows to pass errors and would allow to cancel device_add
in case of error.
Thanks very much for Igor's review and suggestion.

v3:
 -deal with start-up cpus in pc_cpu_plug as Igor suggested.

v2:
 -Add 3 new patches(5/7,6/7,7/7), delete original patch 5/5.
  1/5-->1/7
  2/5-->2/7
  3/5-->3/7
  4/5-->4/7
 Patch 1/7:
 -add errp argument to catch error.
 -return error instead of aborting if cpu id is invalid.
 -make acpi_cpu_plug_cb as a wrapper around AcpiCpuHotplug_add.
 Patch 3/7:
 -remove unused AcpiCpuHotplug_add directly.
 Patch 5/7:
 -switch the last user of cpu hotplug notifier to hotplug handler API, and
  remove the unused cpu hotplug notify.
 Patch 6/7:
 -split the function rename (just cleanup) into single patch.
 Patch 7/7:
 -introduce help function acpi_set_local_sts to keep the bit setting in
  one place.

Gu Zheng (7):
  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
  pc: Update rtc_cmos in pc_cpu_plug
  cpu-hotplug: rename function for better readability
  acpi/cpu-hotplug: introduce help function to keep bit setting in one
    place

 hw/acpi/cpu_hotplug.c         |   35 ++++++++++++++++++++--------
 hw/acpi/ich9.c                |   18 ++++----------
 hw/acpi/piix4.c               |   18 +++-----------
 hw/i386/pc.c                  |   51 +++++++++++++++++++++++++----------------
 include/hw/acpi/cpu_hotplug.h |    7 +++--
 include/hw/acpi/ich9.h        |    1 -
 include/sysemu/sysemu.h       |    3 --
 qom/cpu.c                     |   10 --------
 8 files changed, 69 insertions(+), 74 deletions(-)

-- 
1.7.7

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

[Gu Zheng]

---
v2:
 -add errp argument to catch error.
 -return error instead of aborting if cpu id is invalid.
 -make acpi_cpu_plug_cb as a wrapper around AcpiCpuHotplug_add.
---

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..dfd6de5 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, CPUState *cpu, Error **errp)
+{
+    CPUClass *k = CPU_GET_CLASS(cpu);
+    int64_t cpu_id;
+
+    cpu_id = k->get_arch_id(cpu);
+    if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
+        error_setg(errp, "acpi: invalid cpu id: %" PRIi64, cpu_id);
+        return;
+    }
+
+    AcpiCpuHotplug_add(&ar->gpe, g, cpu);
+
+    acpi_update_sci(ar, irq);
+}
+
 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..166edb0 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, CPUState *dev, Error **errp);
+
 void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu);
 
 void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
-- 
1.7.7

[Qemu-devel] [PATCH V3 2/7] 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         |   14 +++-----------
 include/hw/acpi/ich9.h |    1 -
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 7b14bbb..c53d4ab 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,9 @@ 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,
+                         CPU(dev), errp);
     } 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 V3 3/7] acpi:piix4: convert cpu hotplug handle to hotplug_handler API

[Gu Zheng]

Convert notifier based hotplug handle to hotplug_handler API,
and remove the unused AcpiCpuHotplug_add().

v2:
-remove the unused AcpiCpuHotplug_add().

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

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index dfd6de5..0b9fa55 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -48,22 +48,12 @@ void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
         return;
     }
 
-    AcpiCpuHotplug_add(&ar->gpe, g, cpu);
+    ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS;
+    g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
 
     acpi_update_sci(ar, irq);
 }
 
-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,
                          AcpiCpuHotplug *gpe_cpu, uint16_t base)
 {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b72b34e..b046cd3 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, CPU(dev), errp);
     } 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);
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 166edb0..8188630 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -23,8 +23,6 @@ typedef struct AcpiCpuHotplug {
 void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
                       AcpiCpuHotplug *g, CPUState *dev, Error **errp);
 
-void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu);
-
 void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
                          AcpiCpuHotplug *gpe_cpu, uint16_t base);
 #endif
-- 
1.7.7

[Qemu-devel] [PATCH V3 4/7] 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.

v3:
 -deal with start up cpus in a more neat way as Igor suggested.
v2:
 -just rebase.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b6c9b61..e25e71b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1613,11 +1613,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 (!pcms->acpi_dev) {
+        if (dev->hotplugged) {
+            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);
     }
 }
 
@@ -1626,7 +1649,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);
     }
 
-- 
1.7.7

[Qemu-devel] [PATCH V3 5/7] pc: Update rtc_cmos in pc_cpu_plug

[Gu Zheng]

Update rtc_cmos in pc_cpu_plug directly instead of the notifier, with
this change, there will no user of CPU hot-plug notifier any more, so
remove it.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c            |   25 ++++++-------------------
 include/sysemu/sysemu.h |    3 ---
 qom/cpu.c               |   10 ----------
 3 files changed, 6 insertions(+), 32 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e25e71b..8b887c7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -353,20 +353,7 @@ static void pc_cmos_init_late(void *opaque)
     qemu_unregister_reset(pc_cmos_init_late, opaque);
 }
 
-typedef struct RTCCPUHotplugArg {
-    Notifier cpu_added_notifier;
-    ISADevice *rtc_state;
-} RTCCPUHotplugArg;
-
-static void rtc_notify_cpu_added(Notifier *notifier, void *data)
-{
-    RTCCPUHotplugArg *arg = container_of(notifier, RTCCPUHotplugArg,
-                                         cpu_added_notifier);
-    ISADevice *s = arg->rtc_state;
-
-    /* increment the number of CPUs */
-    rtc_set_memory(s, 0x5f, rtc_get_memory(s, 0x5f) + 1);
-}
+static ISADevice *rtc_state;
 
 void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
                   const char *boot_device,
@@ -376,7 +363,6 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
     int val, nb, i;
     FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
     static pc_cmos_init_late_arg arg;
-    static RTCCPUHotplugArg cpu_hotplug_cb;
 
     /* various important CMOS locations needed by PC/Bochs bios */
 
@@ -415,10 +401,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
 
     /* set the number of CPU */
     rtc_set_memory(s, 0x5f, smp_cpus - 1);
-    /* init CPU hotplug notifier */
-    cpu_hotplug_cb.rtc_state = s;
-    cpu_hotplug_cb.cpu_added_notifier.notify = rtc_notify_cpu_added;
-    qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifier);
+
+    rtc_state = s;
 
     if (set_boot_dev(s, boot_device)) {
         exit(1);
@@ -1630,6 +1614,9 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
 
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+
+    /* increment the number of CPUs */
+    rtc_set_memory(rtc_state, 0x5f, rtc_get_memory(rtc_state, 0x5f) + 1);
 out:
     error_propagate(errp, local_err);
 }
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index d8539fd..acfe494 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -183,9 +183,6 @@ void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
 /* generic hotplug */
 void drive_hot_add(Monitor *mon, const QDict *qdict);
 
-/* CPU hotplug */
-void qemu_register_cpu_added_notifier(Notifier *notifier);
-
 /* pcie aer error injection */
 void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
 int do_pcie_aer_inject_error(Monitor *mon,
diff --git a/qom/cpu.c b/qom/cpu.c
index b32dd0a..19c5de5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -107,15 +107,6 @@ static void cpu_common_get_memory_mapping(CPUState *cpu,
     error_setg(errp, "Obtaining memory mappings is unsupported on this CPU.");
 }
 
-/* CPU hot-plug notifiers */
-static NotifierList cpu_added_notifiers =
-    NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
-
-void qemu_register_cpu_added_notifier(Notifier *notifier)
-{
-    notifier_list_add(&cpu_added_notifiers, notifier);
-}
-
 void cpu_reset_interrupt(CPUState *cpu, int mask)
 {
     cpu->interrupt_request &= ~mask;
@@ -303,7 +294,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 V3 6/7] cpu-hotplug: rename function for better readability

[Gu Zheng]

Rename:
AcpiCpuHotplug_init --> acpi_cpu_hotplug_init
AcpiCpuHotplug_ops --> acpi_cpu_hotplug_ops
for better readability, just cleanup.

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

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 0b9fa55..7629686 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -54,8 +54,8 @@ void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
     acpi_update_sci(ar, irq);
 }
 
-void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
-                         AcpiCpuHotplug *gpe_cpu, uint16_t base)
+void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
+                           AcpiCpuHotplug *gpe_cpu, uint16_t base)
 {
     CPUState *cpu;
 
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index c53d4ab..42cf8fa 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -235,8 +235,8 @@ 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),
-                        &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
+    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) {
         acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b046cd3..205ba66 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -555,8 +555,8 @@ 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,
-                        PIIX4_CPU_HOTPLUG_IO_BASE);
+    acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
+                          PIIX4_CPU_HOTPLUG_IO_BASE);
 
     if (s->acpi_memory_hotplug.is_enabled) {
         acpi_memory_hotplug_init(parent, OBJECT(s), &s->acpi_memory_hotplug);
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 8188630..d015bef 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -23,6 +23,6 @@ typedef struct AcpiCpuHotplug {
 void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
                       AcpiCpuHotplug *g, CPUState *dev, Error **errp);
 
-void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
-                         AcpiCpuHotplug *gpe_cpu, uint16_t base);
+void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
+                           AcpiCpuHotplug *gpe_cpu, uint16_t base);
 #endif
-- 
1.7.7

[Qemu-devel] [PATCH V3 7/7] acpi/cpu-hotplug: introduce help function to keep bit setting in one place

[Gu Zheng]

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 <guz.fnst@cn.fujitsu.com>
---
 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;
     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);
     }
     memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
                           gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
-- 
1.7.7

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

[Gu Zheng]

Hi Igor,
The issues you mentioned in previous comments are fixed in this version.
Could please help to review it?

Thanks,
Gu

On 09/17/2014 09:23 AM, Gu Zheng wrote:

> Previously we use cpu_added_notifiers to register cpu hotplug notifier callback
> which is not able to pass/handle errors, so we switch it to unified hotplug
> handler API which allows to pass errors and would allow to cancel device_add
> in case of error.
> Thanks very much for Igor's review and suggestion.
> 
> v3:
>  -deal with start-up cpus in pc_cpu_plug as Igor suggested.
> 
> v2:
>  -Add 3 new patches(5/7,6/7,7/7), delete original patch 5/5.
>   1/5-->1/7
>   2/5-->2/7
>   3/5-->3/7
>   4/5-->4/7
>  Patch 1/7:
>  -add errp argument to catch error.
>  -return error instead of aborting if cpu id is invalid.
>  -make acpi_cpu_plug_cb as a wrapper around AcpiCpuHotplug_add.
>  Patch 3/7:
>  -remove unused AcpiCpuHotplug_add directly.
>  Patch 5/7:
>  -switch the last user of cpu hotplug notifier to hotplug handler API, and
>   remove the unused cpu hotplug notify.
>  Patch 6/7:
>  -split the function rename (just cleanup) into single patch.
>  Patch 7/7:
>  -introduce help function acpi_set_local_sts to keep the bit setting in
>   one place.
> 
> Gu Zheng (7):
>   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
>   pc: Update rtc_cmos in pc_cpu_plug
>   cpu-hotplug: rename function for better readability
>   acpi/cpu-hotplug: introduce help function to keep bit setting in one
>     place
> 
>  hw/acpi/cpu_hotplug.c         |   35 ++++++++++++++++++++--------
>  hw/acpi/ich9.c                |   18 ++++----------
>  hw/acpi/piix4.c               |   18 +++-----------
>  hw/i386/pc.c                  |   51 +++++++++++++++++++++++++----------------
>  include/hw/acpi/cpu_hotplug.h |    7 +++--
>  include/hw/acpi/ich9.h        |    1 -
>  include/sysemu/sysemu.h       |    3 --
>  qom/cpu.c                     |   10 --------
>  8 files changed, 69 insertions(+), 74 deletions(-)
> 

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

[Gu Zheng]

ping...

On 09/17/2014 09:23 AM, Gu Zheng wrote:

> Previously we use cpu_added_notifiers to register cpu hotplug notifier callback
> which is not able to pass/handle errors, so we switch it to unified hotplug
> handler API which allows to pass errors and would allow to cancel device_add
> in case of error.
> Thanks very much for Igor's review and suggestion.
> 
> v3:
>  -deal with start-up cpus in pc_cpu_plug as Igor suggested.
> 
> v2:
>  -Add 3 new patches(5/7,6/7,7/7), delete original patch 5/5.
>   1/5-->1/7
>   2/5-->2/7
>   3/5-->3/7
>   4/5-->4/7
>  Patch 1/7:
>  -add errp argument to catch error.
>  -return error instead of aborting if cpu id is invalid.
>  -make acpi_cpu_plug_cb as a wrapper around AcpiCpuHotplug_add.
>  Patch 3/7:
>  -remove unused AcpiCpuHotplug_add directly.
>  Patch 5/7:
>  -switch the last user of cpu hotplug notifier to hotplug handler API, and
>   remove the unused cpu hotplug notify.
>  Patch 6/7:
>  -split the function rename (just cleanup) into single patch.
>  Patch 7/7:
>  -introduce help function acpi_set_local_sts to keep the bit setting in
>   one place.
> 
> Gu Zheng (7):
>   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
>   pc: Update rtc_cmos in pc_cpu_plug
>   cpu-hotplug: rename function for better readability
>   acpi/cpu-hotplug: introduce help function to keep bit setting in one
>     place
> 
>  hw/acpi/cpu_hotplug.c         |   35 ++++++++++++++++++++--------
>  hw/acpi/ich9.c                |   18 ++++----------
>  hw/acpi/piix4.c               |   18 +++-----------
>  hw/i386/pc.c                  |   51 +++++++++++++++++++++++++----------------
>  include/hw/acpi/cpu_hotplug.h |    7 +++--
>  include/hw/acpi/ich9.h        |    1 -
>  include/sysemu/sysemu.h       |    3 --
>  qom/cpu.c                     |   10 --------
>  8 files changed, 69 insertions(+), 74 deletions(-)
> 

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

[Igor Mammedov]

On Wed, 17 Sep 2014 09:24:00 +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.
> 
> v3:
>  -deal with start up cpus in a more neat way as Igor suggested.
> v2:
>  -just rebase.
> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c |   26 +++++++++++++++++++++++++-
>  1 files changed, 25 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b6c9b61..e25e71b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1613,11 +1613,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 (!pcms->acpi_dev) {
> +        if (dev->hotplugged) {
> +            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);
above call this would rise SCI unconditionally whether CPU is hotplugged or not.
perhaps checking for this in acpi_cpu_plug_cb() and rising IRQ only for hotplugged
CPUs would be better.

> +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);
>      }
>  }
>  
> @@ -1626,7 +1649,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);
>      }
>  

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

[Igor Mammedov]

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

> ---
> v2:
>  -add errp argument to catch error.
>  -return error instead of aborting if cpu id is invalid.
>  -make acpi_cpu_plug_cb as a wrapper around AcpiCpuHotplug_add.
> ---
> 
> 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..dfd6de5 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, CPUState *cpu, Error **errp)
s/CPUState *cpu/DeviceState *dev/ like it's done for other handlers
and do cast to CPU inside.

> +{
> +    CPUClass *k = CPU_GET_CLASS(cpu);
> +    int64_t cpu_id;
> +
> +    cpu_id = k->get_arch_id(cpu);
> +    if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
> +        error_setg(errp, "acpi: invalid cpu id: %" PRIi64, cpu_id);
> +        return;
> +    }
> +
> +    AcpiCpuHotplug_add(&ar->gpe, g, cpu);
> +
> +    acpi_update_sci(ar, irq);
> +}
> +
>  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..166edb0 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, CPUState *dev, Error **errp);
> +
>  void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu);
>  
>  void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,

Re: [Qemu-devel] [PATCH V3 5/7] pc: Update rtc_cmos in pc_cpu_plug

[Igor Mammedov]

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

> Update rtc_cmos in pc_cpu_plug directly instead of the notifier, with
> this change, there will no user of CPU hot-plug notifier any more, so
> remove it.
> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c            |   25 ++++++-------------------
>  include/sysemu/sysemu.h |    3 ---
>  qom/cpu.c               |   10 ----------
>  3 files changed, 6 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e25e71b..8b887c7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -353,20 +353,7 @@ static void pc_cmos_init_late(void *opaque)
>      qemu_unregister_reset(pc_cmos_init_late, opaque);
>  }
>  
> -typedef struct RTCCPUHotplugArg {
> -    Notifier cpu_added_notifier;
> -    ISADevice *rtc_state;
> -} RTCCPUHotplugArg;
> -
> -static void rtc_notify_cpu_added(Notifier *notifier, void *data)
> -{
> -    RTCCPUHotplugArg *arg = container_of(notifier, RTCCPUHotplugArg,
> -                                         cpu_added_notifier);
> -    ISADevice *s = arg->rtc_state;
> -
> -    /* increment the number of CPUs */
> -    rtc_set_memory(s, 0x5f, rtc_get_memory(s, 0x5f) + 1);
> -}
> +static ISADevice *rtc_state;
It's not recommended to use global variables.

Instead of make link<rtc> property in PCMachine and use it pretty much like acpi_dev
is used in 4/7.

>  
>  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>                    const char *boot_device,
> @@ -376,7 +363,6 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      int val, nb, i;
>      FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>      static pc_cmos_init_late_arg arg;
> -    static RTCCPUHotplugArg cpu_hotplug_cb;
>  
>      /* various important CMOS locations needed by PC/Bochs bios */
>  
> @@ -415,10 +401,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>  
>      /* set the number of CPU */
>      rtc_set_memory(s, 0x5f, smp_cpus - 1);
> -    /* init CPU hotplug notifier */
> -    cpu_hotplug_cb.rtc_state = s;
> -    cpu_hotplug_cb.cpu_added_notifier.notify = rtc_notify_cpu_added;
> -    qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifier);
> +
> +    rtc_state = s;
>  
>      if (set_boot_dev(s, boot_device)) {
>          exit(1);
> @@ -1630,6 +1614,9 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>  
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> +
> +    /* increment the number of CPUs */
> +    rtc_set_memory(rtc_state, 0x5f, rtc_get_memory(rtc_state, 0x5f) + 1);
looks wrong, what if error happens in plug() handler???

>  out:
>      error_propagate(errp, local_err);
>  }
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index d8539fd..acfe494 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -183,9 +183,6 @@ void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
>  /* generic hotplug */
>  void drive_hot_add(Monitor *mon, const QDict *qdict);
>  
> -/* CPU hotplug */
> -void qemu_register_cpu_added_notifier(Notifier *notifier);
> -
>  /* pcie aer error injection */
>  void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
>  int do_pcie_aer_inject_error(Monitor *mon,
> diff --git a/qom/cpu.c b/qom/cpu.c
> index b32dd0a..19c5de5 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -107,15 +107,6 @@ static void cpu_common_get_memory_mapping(CPUState *cpu,
>      error_setg(errp, "Obtaining memory mappings is unsupported on this CPU.");
>  }
>  
> -/* CPU hot-plug notifiers */
> -static NotifierList cpu_added_notifiers =
> -    NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
> -
> -void qemu_register_cpu_added_notifier(Notifier *notifier)
> -{
> -    notifier_list_add(&cpu_added_notifiers, notifier);
> -}
> -
>  void cpu_reset_interrupt(CPUState *cpu, int mask)
>  {
>      cpu->interrupt_request &= ~mask;
> @@ -303,7 +294,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);
>      }
>  }

Re: [Qemu-devel] [PATCH V3 5/7] pc: Update rtc_cmos in pc_cpu_plug

[Igor Mammedov]

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

> Update rtc_cmos in pc_cpu_plug directly instead of the notifier, with
> this change, there will no user of CPU hot-plug notifier any more, so
> remove it.
In addition move removal of unused notifier in separate patch, please.

> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c            |   25 ++++++-------------------
>  include/sysemu/sysemu.h |    3 ---
>  qom/cpu.c               |   10 ----------
>  3 files changed, 6 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e25e71b..8b887c7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -353,20 +353,7 @@ static void pc_cmos_init_late(void *opaque)
>      qemu_unregister_reset(pc_cmos_init_late, opaque);
>  }
>  
> -typedef struct RTCCPUHotplugArg {
> -    Notifier cpu_added_notifier;
> -    ISADevice *rtc_state;
> -} RTCCPUHotplugArg;
> -
> -static void rtc_notify_cpu_added(Notifier *notifier, void *data)
> -{
> -    RTCCPUHotplugArg *arg = container_of(notifier, RTCCPUHotplugArg,
> -                                         cpu_added_notifier);
> -    ISADevice *s = arg->rtc_state;
> -
> -    /* increment the number of CPUs */
> -    rtc_set_memory(s, 0x5f, rtc_get_memory(s, 0x5f) + 1);
> -}
> +static ISADevice *rtc_state;
>  
>  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>                    const char *boot_device,
> @@ -376,7 +363,6 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      int val, nb, i;
>      FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>      static pc_cmos_init_late_arg arg;
> -    static RTCCPUHotplugArg cpu_hotplug_cb;
>  
>      /* various important CMOS locations needed by PC/Bochs bios */
>  
> @@ -415,10 +401,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>  
>      /* set the number of CPU */
>      rtc_set_memory(s, 0x5f, smp_cpus - 1);
> -    /* init CPU hotplug notifier */
> -    cpu_hotplug_cb.rtc_state = s;
> -    cpu_hotplug_cb.cpu_added_notifier.notify = rtc_notify_cpu_added;
> -    qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifier);
> +
> +    rtc_state = s;
>  
>      if (set_boot_dev(s, boot_device)) {
>          exit(1);
> @@ -1630,6 +1614,9 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>  
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> +
> +    /* increment the number of CPUs */
> +    rtc_set_memory(rtc_state, 0x5f, rtc_get_memory(rtc_state, 0x5f) + 1);
>  out:
>      error_propagate(errp, local_err);
>  }
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index d8539fd..acfe494 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -183,9 +183,6 @@ void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
>  /* generic hotplug */
>  void drive_hot_add(Monitor *mon, const QDict *qdict);
>  
> -/* CPU hotplug */
> -void qemu_register_cpu_added_notifier(Notifier *notifier);
> -
>  /* pcie aer error injection */
>  void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
>  int do_pcie_aer_inject_error(Monitor *mon,
> diff --git a/qom/cpu.c b/qom/cpu.c
> index b32dd0a..19c5de5 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -107,15 +107,6 @@ static void cpu_common_get_memory_mapping(CPUState *cpu,
>      error_setg(errp, "Obtaining memory mappings is unsupported on this CPU.");
>  }
>  
> -/* CPU hot-plug notifiers */
> -static NotifierList cpu_added_notifiers =
> -    NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
> -
> -void qemu_register_cpu_added_notifier(Notifier *notifier)
> -{
> -    notifier_list_add(&cpu_added_notifiers, notifier);
> -}
> -
>  void cpu_reset_interrupt(CPUState *cpu, int mask)
>  {
>      cpu->interrupt_request &= ~mask;
> @@ -303,7 +294,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);
>      }
>  }

Re: [Qemu-devel] [PATCH V3 7/7] acpi/cpu-hotplug: introduce help function to keep bit setting in one place

[Igor Mammedov]

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

>      }
>      memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
>                            gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);

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

[Gu Zheng]

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

> On Wed, 17 Sep 2014 09:24:00 +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.
>>
>> v3:
>>  -deal with start up cpus in a more neat way as Igor suggested.
>> v2:
>>  -just rebase.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  hw/i386/pc.c |   26 +++++++++++++++++++++++++-
>>  1 files changed, 25 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index b6c9b61..e25e71b 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1613,11 +1613,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 (!pcms->acpi_dev) {
>> +        if (dev->hotplugged) {
>> +            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);
> above call this would rise SCI unconditionally whether CPU is hotplugged or not.

Above precheck can avoid this, startup CPUs won't run into this: 
    if (!pcms->acpi_dev) {
        if (dev->hotplugged) {
            error_setg(&local_err,
                       "cpu hotplug is not enabled: missing acpi device");
        }
        goto out;
    }

Thanks,
Gu

> perhaps checking for this in acpi_cpu_plug_cb() and rising IRQ only for hotplugged
> CPUs would be better.
> 
>> +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);
>>      }
>>  }
>>  
>> @@ -1626,7 +1649,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);
>>      }
>>  
> 
> .
> 

Re: [Qemu-devel] [PATCH V3 5/7] pc: Update rtc_cmos in pc_cpu_plug

[Gu Zheng]

Hi Igor,
On 09/26/2014 09:23 PM, Igor Mammedov wrote:

> On Wed, 17 Sep 2014 09:24:01 +0800
> Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> 
>> Update rtc_cmos in pc_cpu_plug directly instead of the notifier, with
>> this change, there will no user of CPU hot-plug notifier any more, so
>> remove it.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  hw/i386/pc.c            |   25 ++++++-------------------
>>  include/sysemu/sysemu.h |    3 ---
>>  qom/cpu.c               |   10 ----------
>>  3 files changed, 6 insertions(+), 32 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index e25e71b..8b887c7 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -353,20 +353,7 @@ static void pc_cmos_init_late(void *opaque)
>>      qemu_unregister_reset(pc_cmos_init_late, opaque);
>>  }
>>  
>> -typedef struct RTCCPUHotplugArg {
>> -    Notifier cpu_added_notifier;
>> -    ISADevice *rtc_state;
>> -} RTCCPUHotplugArg;
>> -
>> -static void rtc_notify_cpu_added(Notifier *notifier, void *data)
>> -{
>> -    RTCCPUHotplugArg *arg = container_of(notifier, RTCCPUHotplugArg,
>> -                                         cpu_added_notifier);
>> -    ISADevice *s = arg->rtc_state;
>> -
>> -    /* increment the number of CPUs */
>> -    rtc_set_memory(s, 0x5f, rtc_get_memory(s, 0x5f) + 1);
>> -}
>> +static ISADevice *rtc_state;
> It's not recommended to use global variables.
> 
> Instead of make link<rtc> property in PCMachine and use it pretty much like acpi_dev
> is used in 4/7.

Right, this way is more reasonable and neat.

> 
>>  
>>  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>                    const char *boot_device,
>> @@ -376,7 +363,6 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>      int val, nb, i;
>>      FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>>      static pc_cmos_init_late_arg arg;
>> -    static RTCCPUHotplugArg cpu_hotplug_cb;
>>  
>>      /* various important CMOS locations needed by PC/Bochs bios */
>>  
>> @@ -415,10 +401,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>  
>>      /* set the number of CPU */
>>      rtc_set_memory(s, 0x5f, smp_cpus - 1);
>> -    /* init CPU hotplug notifier */
>> -    cpu_hotplug_cb.rtc_state = s;
>> -    cpu_hotplug_cb.cpu_added_notifier.notify = rtc_notify_cpu_added;
>> -    qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifier);
>> +
>> +    rtc_state = s;
>>  
>>      if (set_boot_dev(s, boot_device)) {
>>          exit(1);
>> @@ -1630,6 +1614,9 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>>  
>>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>> +
>> +    /* increment the number of CPUs */
>> +    rtc_set_memory(rtc_state, 0x5f, rtc_get_memory(rtc_state, 0x5f) + 1);
> looks wrong, what if error happens in plug() handler???

Ah, yes, we need to confirm the plug cb result before update rtc_state.

Thanks,
Gu

> 
>>  out:
>>      error_propagate(errp, local_err);
>>  }
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index d8539fd..acfe494 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -183,9 +183,6 @@ void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
>>  /* generic hotplug */
>>  void drive_hot_add(Monitor *mon, const QDict *qdict);
>>  
>> -/* CPU hotplug */
>> -void qemu_register_cpu_added_notifier(Notifier *notifier);
>> -
>>  /* pcie aer error injection */
>>  void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
>>  int do_pcie_aer_inject_error(Monitor *mon,
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index b32dd0a..19c5de5 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -107,15 +107,6 @@ static void cpu_common_get_memory_mapping(CPUState *cpu,
>>      error_setg(errp, "Obtaining memory mappings is unsupported on this CPU.");
>>  }
>>  
>> -/* CPU hot-plug notifiers */
>> -static NotifierList cpu_added_notifiers =
>> -    NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
>> -
>> -void qemu_register_cpu_added_notifier(Notifier *notifier)
>> -{
>> -    notifier_list_add(&cpu_added_notifiers, notifier);
>> -}
>> -
>>  void cpu_reset_interrupt(CPUState *cpu, int mask)
>>  {
>>      cpu->interrupt_request &= ~mask;
>> @@ -303,7 +294,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);
>>      }
>>  }
> 
> .
> 

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

[Igor Mammedov]

On Mon, 29 Sep 2014 10:58:04 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> Hi Igor,
> On 09/26/2014 09:06 PM, Igor Mammedov wrote:
> 
> > On Wed, 17 Sep 2014 09:24:00 +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.
> >>
> >> v3:
> >>  -deal with start up cpus in a more neat way as Igor suggested.
> >> v2:
> >>  -just rebase.
> >>
> >> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> >> ---
> >>  hw/i386/pc.c |   26 +++++++++++++++++++++++++-
> >>  1 files changed, 25 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index b6c9b61..e25e71b 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1613,11 +1613,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 (!pcms->acpi_dev) {
> >> +        if (dev->hotplugged) {
> >> +            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);
> > above call this would rise SCI unconditionally whether CPU is
> > hotplugged or not.
> 
> Above precheck can avoid this, startup CPUs won't run into this: 
>     if (!pcms->acpi_dev) {
>         if (dev->hotplugged) {
>             error_setg(&local_err,
>                        "cpu hotplug is not enabled: missing acpi
> device"); }
>         goto out;
>     }
that's true, but it robs plug handler of initializing startup CPUs'
state in acpi_dev and leads to code duplication between
AcpiCpuHotplug_init() and acpi_cpu_plug_cb().

> 
> Thanks,
> Gu
> 
> > perhaps checking for this in acpi_cpu_plug_cb() and rising IRQ only
> > for hotplugged CPUs would be better.
> > 
> >> +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);
> >>      }
> >>  }
> >>  
> >> @@ -1626,7 +1649,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);
> >>      }
> >>  
> > 
> > .
> > 
> 
> 

Re: [Qemu-devel] [PATCH V3 7/7] acpi/cpu-hotplug: introduce help function to keep bit setting in one place

[Gu Zheng]

Hi Igor,
On 09/26/2014 09:37 PM, Igor Mammedov wrote:

> On Wed, 17 Sep 2014 09:24:03 +0800
> Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
>> ---
>>  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);
> 
> .
> 

Re: [Qemu-devel] [PATCH V3 7/7] acpi/cpu-hotplug: introduce help function to keep bit setting in one place

[Gu Zheng]

On 09/29/2014 05:47 PM, Igor Mammedov wrote:

> On Mon, 29 Sep 2014 17:22:08 +0800
> Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> 
>> Hi Igor,
>> On 09/26/2014 09:37 PM, Igor Mammedov wrote:
>>
>>> On Wed, 17 Sep 2014 09:24:03 +0800
>>> Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
>>>> ---
>>>>  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?
> Could you point out what dependecies are there?

E.g.
Allocation smi_irq for PIIX4PMState dependents on the valid *first_cpu*.

smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
/* TODO: Populate SPD eeprom data.  */
smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
                      gsi[9], *smi_irq,
                      kvm_enabled(), fw_cfg, &piix4_pm);

Thanks,
Gu

> 
>>
>> Thanks,
>> Gu
>>
>>>
>>>>      }
>>>>      memory_region_init_io(&gpe_cpu->io, owner,
>>>> &AcpiCpuHotplug_ops, gpe_cpu, "acpi-cpu-hotplug",
>>>> ACPI_GPE_PROC_LEN);
>>>
>>> .
>>>
>>
>>
> 
> .
> 

Re: [Qemu-devel] [PATCH V3 7/7] acpi/cpu-hotplug: introduce help function to keep bit setting in one place

[Igor Mammedov]

On Mon, 29 Sep 2014 17:22:08 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> Hi Igor,
> On 09/26/2014 09:37 PM, Igor Mammedov wrote:
> 
> > On Wed, 17 Sep 2014 09:24:03 +0800
> > Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
> >> ---
> >>  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?
Could you point out what dependecies are there?

> 
> Thanks,
> Gu
> 
> > 
> >>      }
> >>      memory_region_init_io(&gpe_cpu->io, owner,
> >> &AcpiCpuHotplug_ops, gpe_cpu, "acpi-cpu-hotplug",
> >> ACPI_GPE_PROC_LEN);
> > 
> > .
> > 
> 
> 

Re: [Qemu-devel] [PATCH V3 7/7] acpi/cpu-hotplug: introduce help function to keep bit setting in one place

[Igor Mammedov]

On Mon, 29 Sep 2014 17:44:27 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> On 09/29/2014 05:47 PM, Igor Mammedov wrote:
> 
> > On Mon, 29 Sep 2014 17:22:08 +0800
> > Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> > 
> >> Hi Igor,
> >> On 09/26/2014 09:37 PM, Igor Mammedov wrote:
> >>
> >>> On Wed, 17 Sep 2014 09:24:03 +0800
> >>> Gu Zheng <guz.fnst@cn.fujitsu.com> 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 <guz.fnst@cn.fujitsu.com>
> >>>> ---
> >>>>  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?
> > Could you point out what dependecies are there?
> 
> E.g.
> Allocation smi_irq for PIIX4PMState dependents on the valid
> *first_cpu*.
> 
> smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
> /* TODO: Populate SPD eeprom data.  */
> smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
>                       gsi[9], *smi_irq,
>                       kvm_enabled(), fw_cfg, &piix4_pm);
Looks like it would be too much trouble for not much gain.
Lets keep this patch as is.

> Thanks,
> Gu
> 
> > 
> >>
> >> Thanks,
> >> Gu
> >>
> >>>
> >>>>      }
> >>>>      memory_region_init_io(&gpe_cpu->io, owner,
> >>>> &AcpiCpuHotplug_ops, gpe_cpu, "acpi-cpu-hotplug",
> >>>> ACPI_GPE_PROC_LEN);
> >>>
> >>> .
> >>>
> >>
> >>
> > 
> > .
> > 
> 
> 
>