[Qemu-devel] [PATCH 0/7] Add CPU hot-plug to qemu (pc only). v2

[Qemu-devel] [PATCH 0/7] Add CPU hot-plug to qemu (pc only). v2

[Igor Mammedov]

Previous version was titled "Make vcpu hotplug work for qemu"
	
Why it is needed?
  Feature is implemented by many other solutions and seems to be in high
  demand from users. For example it's possible to do hot-plug with xen
  tool stack or with kvm using qemu-kvm+some voodoo (i.e. lost patches).
  But it's not possible to do it with mainline qemu, and this is attempt
  to start fixing it.

This patch-set lays basic infrastructure for cpu hot-plug and introduces
only cpu hot-add for now. This will provide skeleton for cpu hot-unplug
patches that are posted against qemu-kvm.

v2 changes:
   - small fixes in [1/7], change list in the patch
   - got rid of cpu_set command
   - converted pc cpu to qdev device, now it's possible to add cpu using
     monitor command 'device_add'

Signed-off-by: Igor Mammedov <imammedo@redhat.com>

[Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Igor Mammedov]

Introduce a new structure CPUS as the controller of ICC (INTERRUPT
CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
of sysbus. So we can support APIC hot-plug feature.

This is repost of original patch for qemu-kvm rebased on current qemu:
http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
All credits to Liu Ping Fan for writing it.

V2 changes:
  - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
    we got as input param instead for qbus_create, this makes cpus
    apics visible in "info qtree" monitor command
  - fix format error spotted by Jan and missed by checkpatch
  - cpu_has_apic_feature: return bool instead of int

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 Makefile.objs       |    2 +-
 hw/apic.c           |   24 ++++++++---
 hw/apic.h           |    1 +
 hw/icc_bus.c        |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/icc_bus.h        |   41 ++++++++++++++++++
 hw/pc.c             |   14 +++++--
 target-i386/cpu.h   |    1 +
 target-i386/cpuid.c |   12 +++++
 8 files changed, 196 insertions(+), 12 deletions(-)
 create mode 100644 hw/icc_bus.c
 create mode 100644 hw/icc_bus.h

diff --git a/Makefile.objs b/Makefile.objs
index 4f6d26c..45df666 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -212,7 +212,7 @@ hw-obj-$(CONFIG_USB_UHCI) += usb-uhci.o
 hw-obj-$(CONFIG_USB_OHCI) += usb-ohci.o
 hw-obj-$(CONFIG_USB_EHCI) += usb-ehci.o
 hw-obj-$(CONFIG_FDC) += fdc.o
-hw-obj-$(CONFIG_ACPI) += acpi.o acpi_piix4.o
+hw-obj-$(CONFIG_ACPI) += acpi.o acpi_piix4.o icc_bus.o
 hw-obj-$(CONFIG_APM) += pm_smbus.o apm.o
 hw-obj-$(CONFIG_DMA) += dma.o
 hw-obj-$(CONFIG_HPET) += hpet.o
diff --git a/hw/apic.c b/hw/apic.c
index 9d0f460..e666b4c 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -21,9 +21,10 @@
 #include "ioapic.h"
 #include "qemu-timer.h"
 #include "host-utils.h"
-#include "sysbus.h"
+#include "icc_bus.h"
 #include "trace.h"
 #include "pc.h"
+#include "exec-memory.h"
 
 /* APIC Local Vector Table */
 #define APIC_LVT_TIMER   0
@@ -80,7 +81,7 @@
 typedef struct APICState APICState;
 
 struct APICState {
-    SysBusDevice busdev;
+    ICCBusDevice busdev;
     MemoryRegion io_memory;
     void *cpu_env;
     uint32_t apicbase;
@@ -988,9 +989,19 @@ static const MemoryRegionOps apic_io_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int apic_init1(SysBusDevice *dev)
+int apic_mmio_map(DeviceState *dev, target_phys_addr_t base)
 {
-    APICState *s = FROM_SYSBUS(APICState, dev);
+    APICState *s = DO_UPCAST(APICState, busdev.qdev, dev);
+
+    memory_region_add_subregion(get_system_memory(),
+                                base,
+                                &s->io_memory);
+    return 0;
+}
+
+static int apic_init1(ICCBusDevice *dev)
+{
+    APICState *s = DO_UPCAST(APICState, busdev, dev);
     static int last_apic_idx;
 
     if (last_apic_idx >= MAX_APICS) {
@@ -998,7 +1009,6 @@ static int apic_init1(SysBusDevice *dev)
     }
     memory_region_init_io(&s->io_memory, &apic_io_ops, s, "apic",
                           MSI_ADDR_SIZE);
-    sysbus_init_mmio(dev, &s->io_memory);
 
     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
     s->idx = last_apic_idx++;
@@ -1006,7 +1016,7 @@ static int apic_init1(SysBusDevice *dev)
     return 0;
 }
 
-static SysBusDeviceInfo apic_info = {
+static ICCBusDeviceInfo apic_info = {
     .init = apic_init1,
     .qdev.name = "apic",
     .qdev.size = sizeof(APICState),
@@ -1022,7 +1032,7 @@ static SysBusDeviceInfo apic_info = {
 
 static void apic_register_devices(void)
 {
-    sysbus_register_withprop(&apic_info);
+    iccbus_register_devinfo(&apic_info);
 }
 
 device_init(apic_register_devices)
diff --git a/hw/apic.h b/hw/apic.h
index a5c910f..d42081e 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -17,6 +17,7 @@ void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
 uint8_t cpu_get_apic_tpr(DeviceState *s);
 void apic_init_reset(DeviceState *s);
 void apic_sipi(DeviceState *s);
+int apic_mmio_map(DeviceState *dev, target_phys_addr_t base);
 
 /* pc.c */
 int cpu_is_bsp(CPUState *env);
diff --git a/hw/icc_bus.c b/hw/icc_bus.c
new file mode 100644
index 0000000..8a3b1ee
--- /dev/null
+++ b/hw/icc_bus.c
@@ -0,0 +1,113 @@
+/* icc_bus.c
+ * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#include "icc_bus.h"
+
+typedef struct CPUSockets CPUSockets;
+typedef struct ICCBus ICCBus;
+typedef struct ICCBusInfo ICCBusInfo;
+
+struct CPUSockets {
+    SysBusDevice sysdev;
+    ICCBus *icc_bus;
+};
+
+struct CPUSInfo {
+    DeviceInfo info;
+};
+
+struct ICCBus {
+    BusState qbus;
+};
+
+struct ICCBusInfo {
+    BusInfo qinfo;
+};
+
+static CPUSockets *cpu_sockets;
+
+static ICCBusInfo icc_bus_info = {
+    .qinfo.name = "icc",
+    .qinfo.size = sizeof(ICCBus),
+    .qinfo.props = (Property[]) {
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static int iccbus_device_init(DeviceState *dev, DeviceInfo *base)
+{
+    ICCBusDeviceInfo *info = container_of(base, ICCBusDeviceInfo, qdev);
+    ICCBusDevice *idev = DO_UPCAST(ICCBusDevice, qdev, dev);
+
+    return info->init(idev);
+}
+
+void iccbus_register_devinfo(ICCBusDeviceInfo *info)
+{
+    info->qdev.init = iccbus_device_init;
+    info->qdev.bus_info = &icc_bus_info.qinfo;
+    assert(info->qdev.size >= sizeof(ICCBusDevice));
+    qdev_register(&info->qdev);
+}
+
+BusState *get_icc_bus(void)
+{
+    return &cpu_sockets->icc_bus->qbus;
+}
+
+/*Must be called before vcpu's creation*/
+static int cpusockets_init(SysBusDevice *dev)
+{
+    CPUSockets *cpus = DO_UPCAST(CPUSockets, sysdev, dev);
+    BusState *b = qbus_create(&icc_bus_info.qinfo,
+                              &cpus->sysdev.qdev,
+                              "icc");
+    if (b == NULL) {
+        return -1;
+    }
+    b->allow_hotplug = 1; /* Yes, we can */
+    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
+    cpu_sockets = cpus;
+    return 0;
+
+}
+
+static const VMStateDescription vmstate_cpusockets = {
+    .name = "cpusockets",
+    .version_id = 1,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static SysBusDeviceInfo cpusockets_info = {
+    .init = cpusockets_init,
+    .qdev.name = "cpusockets",
+    .qdev.size = sizeof(CPUSockets),
+    .qdev.vmsd = &vmstate_cpusockets,
+    .qdev.reset = NULL,
+    .qdev.no_user = 1,
+};
+
+static void cpusockets_register_devices(void)
+{
+    sysbus_register_withprop(&cpusockets_info);
+}
+
+device_init(cpusockets_register_devices)
diff --git a/hw/icc_bus.h b/hw/icc_bus.h
new file mode 100644
index 0000000..f817873
--- /dev/null
+++ b/hw/icc_bus.h
@@ -0,0 +1,41 @@
+/* ICCBus.h
+ * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+#ifndef QEMU_ICC_H
+#define QEMU_ICC_H
+
+#include "qdev.h"
+#include "sysbus.h"
+
+struct ICCBusDevice {
+    DeviceState qdev;
+};
+typedef struct ICCBusDevice ICCBusDevice;
+typedef int (*iccbus_initfn)(ICCBusDevice *dev);
+
+struct ICCBusDeviceInfo {
+    DeviceInfo qdev;
+    iccbus_initfn init;
+};
+typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
+
+BusState *get_icc_bus(void);
+
+void iccbus_register_devinfo(ICCBusDeviceInfo *info);
+
+#endif
diff --git a/hw/pc.c b/hw/pc.c
index 85304cf..33d8090 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -24,6 +24,7 @@
 #include "hw.h"
 #include "pc.h"
 #include "apic.h"
+#include "icc_bus.h"
 #include "fdc.h"
 #include "ide.h"
 #include "pci.h"
@@ -878,21 +879,21 @@ DeviceState *cpu_get_current_apic(void)
 static DeviceState *apic_init(void *env, uint8_t apic_id)
 {
     DeviceState *dev;
-    SysBusDevice *d;
+    BusState *b;
     static int apic_mapped;
 
-    dev = qdev_create(NULL, "apic");
+    b = get_icc_bus();
+    dev = qdev_create(b, "apic");
     qdev_prop_set_uint8(dev, "id", apic_id);
     qdev_prop_set_ptr(dev, "cpu_env", env);
     qdev_init_nofail(dev);
-    d = sysbus_from_qdev(dev);
 
     /* XXX: mapping more APICs at the same memory location */
     if (apic_mapped == 0) {
         /* NOTE: the APIC is directly connected to the CPU - it is not
            on the global memory bus. */
         /* XXX: what if the base changes? */
-        sysbus_mmio_map(d, 0, MSI_ADDR_BASE);
+        apic_mmio_map(dev, MSI_ADDR_BASE);
         apic_mapped = 1;
     }
 
@@ -959,6 +960,11 @@ void pc_cpus_init(const char *cpu_model)
 #endif
     }
 
+    if (cpu_has_apic_feature(cpu_model) || smp_cpus > 1) {
+        DeviceState *d = qdev_create(NULL, "cpusockets");
+        qdev_init_nofail(d);
+    }
+
     for(i = 0; i < smp_cpus; i++) {
         pc_new_cpu(cpu_model);
     }
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 37dde79..7e66bcf 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -892,6 +892,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
 int cpu_x86_register (CPUX86State *env, const char *cpu_model);
+bool cpu_has_apic_feature(const char *cpu_model);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 91a104b..1664c6c 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -850,6 +850,18 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
     }
 }
 
+bool cpu_has_apic_feature(const char *cpu_model)
+{
+    x86_def_t def;
+
+    memset(&def, 0, sizeof(def));
+
+    if (cpu_x86_find_by_name(&def, cpu_model) < 0) {
+        return false;
+    }
+    return def.features & CPUID_APIC;
+}
+
 int cpu_x86_register (CPUX86State *env, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Jan Kiszka]

On 2012-02-16 00:16, Igor Mammedov wrote:
> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
> of sysbus. So we can support APIC hot-plug feature.
> 
> This is repost of original patch for qemu-kvm rebased on current qemu:
> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
> All credits to Liu Ping Fan for writing it.
> 
> V2 changes:
>   - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>     we got as input param instead for qbus_create, this makes cpus
>     apics visible in "info qtree" monitor command
>   - fix format error spotted by Jan and missed by checkpatch
>   - cpu_has_apic_feature: return bool instead of int
> 

This patch surely no longer applies. And the ICC requires QOM conversion.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Anthony Liguori]

On 02/16/2012 05:25 AM, Jan Kiszka wrote:
> On 2012-02-16 00:16, Igor Mammedov wrote:
>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>> of sysbus. So we can support APIC hot-plug feature.
>>
>> This is repost of original patch for qemu-kvm rebased on current qemu:
>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>> All credits to Liu Ping Fan for writing it.
>>
>> V2 changes:
>>    - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>      we got as input param instead for qbus_create, this makes cpus
>>      apics visible in "info qtree" monitor command
>>    - fix format error spotted by Jan and missed by checkpatch
>>    - cpu_has_apic_feature: return bool instead of int
>>
>
> This patch surely no longer applies. And the ICC requires QOM conversion.

Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.

The LAPIC can be made a child of the CPU device with a bidirectional link.

I would simply create a fixed set of CPU links<> hung off of /devices somewhere 
and use that as the hotplug mechanism.  This matches well the way we model this 
to the guest (we expose a fixed number of pluggable sockets).

Regards,

Anthony Liguori

>
> Jan
>

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Jan Kiszka]

On 2012-02-16 13:42, Anthony Liguori wrote:
> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>> of sysbus. So we can support APIC hot-plug feature.
>>>
>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>> All credits to Liu Ping Fan for writing it.
>>>
>>> V2 changes:
>>>    - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>>      we got as input param instead for qbus_create, this makes cpus
>>>      apics visible in "info qtree" monitor command
>>>    - fix format error spotted by Jan and missed by checkpatch
>>>    - cpu_has_apic_feature: return bool instead of int
>>>
>>
>> This patch surely no longer applies. And the ICC requires QOM conversion.
> 
> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
> 
> The LAPIC can be made a child of the CPU device with a bidirectional link.

We do have a bus here, the IO-APICs are attached to it as well. But we
can surely save that dummy cpusockets device.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Anthony Liguori]

On 02/16/2012 06:50 AM, Jan Kiszka wrote:
> On 2012-02-16 13:42, Anthony Liguori wrote:
>> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>>> of sysbus. So we can support APIC hot-plug feature.
>>>>
>>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>>> All credits to Liu Ping Fan for writing it.
>>>>
>>>> V2 changes:
>>>>     - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>>>       we got as input param instead for qbus_create, this makes cpus
>>>>       apics visible in "info qtree" monitor command
>>>>     - fix format error spotted by Jan and missed by checkpatch
>>>>     - cpu_has_apic_feature: return bool instead of int
>>>>
>>>
>>> This patch surely no longer applies. And the ICC requires QOM conversion.
>>
>> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
>>
>> The LAPIC can be made a child of the CPU device with a bidirectional link.
>
> We do have a bus here, the IO-APICs are attached to it as well.


Isn't that a bus protocol that's specific to the LAPICs/IO-APICs?  I don't think 
that you would logically model the CPU as part of it.

I wonder if modeling inter-lapic communication via a bus is a bit overkill 
compared to what we do today (just a for() loop when needed).

Regards,

Anthony Liguori

  But we
> can surely save that dummy cpusockets device.
>
> Jan
>

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Jan Kiszka]

On 2012-02-16 13:59, Anthony Liguori wrote:
> On 02/16/2012 06:50 AM, Jan Kiszka wrote:
>> On 2012-02-16 13:42, Anthony Liguori wrote:
>>> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>>>> of sysbus. So we can support APIC hot-plug feature.
>>>>>
>>>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>>>> All credits to Liu Ping Fan for writing it.
>>>>>
>>>>> V2 changes:
>>>>>     - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>>>>       we got as input param instead for qbus_create, this makes cpus
>>>>>       apics visible in "info qtree" monitor command
>>>>>     - fix format error spotted by Jan and missed by checkpatch
>>>>>     - cpu_has_apic_feature: return bool instead of int
>>>>>
>>>>
>>>> This patch surely no longer applies. And the ICC requires QOM conversion.
>>>
>>> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
>>>
>>> The LAPIC can be made a child of the CPU device with a bidirectional link.
>>
>> We do have a bus here, the IO-APICs are attached to it as well.
> 
> 
> Isn't that a bus protocol that's specific to the LAPICs/IO-APICs?  I don't think 
> that you would logically model the CPU as part of it.

Actually, MSIs go this way as well.

> 
> I wonder if modeling inter-lapic communication via a bus is a bit overkill 
> compared to what we do today (just a for() loop when needed).

For sure, we can keep such loops for all those use case. But Andreas may
have a different vision as he wanted to eliminate (public) first_cpu
references.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Igor Mammedov]

On 02/16/2012 01:42 PM, Anthony Liguori wrote:
> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>> of sysbus. So we can support APIC hot-plug feature.
>>>
>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>> All credits to Liu Ping Fan for writing it.
>>>
>>> V2 changes:
>>> - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>> we got as input param instead for qbus_create, this makes cpus
>>> apics visible in "info qtree" monitor command
>>> - fix format error spotted by Jan and missed by checkpatch
>>> - cpu_has_apic_feature: return bool instead of int
>>>
>>
>> This patch surely no longer applies. And the ICC requires QOM conversion.
>
> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
>
> The LAPIC can be made a child of the CPU device with a bidirectional link.
>
> I would simply create a fixed set of CPU links<> hung off of /devices somewhere and use that as the hotplug mechanism. This matches well the way we
> model this to the guest (we expose a fixed number of pluggable sockets).

I've just QOM-ified it, but in light of what you just said it may be ignored.
ICC bus was used on pre Pentium 4 smp systems. And whole thing with introducing
it was to provide hot-plugable bus for cpus, since hot-plug on sysbus is disabled
and people argued that sysbus shouldn't be hot-plugable. However it depends on
what we choose to model, we can use pre P4 ICC bus for inter-apic/ioapic communications
or use P4 model allowing hot-plug on sysbus and use it for inter-apic/ioapic
communications if needed.

So I'd rather drop ICC patch and try your approach with CPU links<>, I see no
point in introducing new bus providing we have an alternative model and existing
bus for the task.

>
> Regards,
>
> Anthony Liguori
>
>>
>> Jan
>>
>
>

-- 
Thanks,
  Igor

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Igor Mammedov]

On 02/17/2012 06:02 PM, Igor Mammedov wrote:
> On 02/16/2012 01:42 PM, Anthony Liguori wrote:
>> On 02/16/2012 05:25 AM, Jan Kiszka wrote:
>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
>>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
>>>> of sysbus. So we can support APIC hot-plug feature.
>>>>
>>>> This is repost of original patch for qemu-kvm rebased on current qemu:
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01478.html
>>>> All credits to Liu Ping Fan for writing it.
>>>>
>>>> V2 changes:
>>>> - cpusockets_init: cpu_sockets is not yet initialized, use cpus that
>>>> we got as input param instead for qbus_create, this makes cpus
>>>> apics visible in "info qtree" monitor command
>>>> - fix format error spotted by Jan and missed by checkpatch
>>>> - cpu_has_apic_feature: return bool instead of int
>>>>
>>>
>>> This patch surely no longer applies. And the ICC requires QOM conversion.
>>
>> Also, post-QOM, I don't think having an ICC bus makes a whole lot of sense.
>>
>> The LAPIC can be made a child of the CPU device with a bidirectional link.
>>
>> I would simply create a fixed set of CPU links<> hung off of /devices somewhere and use that as the hotplug mechanism. This matches well the way we
>> model this to the guest (we expose a fixed number of pluggable sockets).
>
> I've just QOM-ified it, but in light of what you just said it may be ignored.
> ICC bus was used on pre Pentium 4 smp systems. And whole thing with introducing
> it was to provide hot-plugable bus for cpus, since hot-plug on sysbus is disabled
> and people argued that sysbus shouldn't be hot-plugable. However it depends on
> what we choose to model, we can use pre P4 ICC bus for inter-apic/ioapic communications
> or use P4 model allowing hot-plug on sysbus and use it for inter-apic/ioapic
> communications if needed.
>
> So I'd rather drop ICC patch and try your approach with CPU links<>, I see no
> point in introducing new bus providing we have an alternative model and existing
> bus for the task.

I've looked at device_add command and qdev_device_add it uses for doing actual work
and in current state it requires (based on Andreas' qom_cpu branch):

For approach where apic and cpu hot-plugged to sysbus.
   1. created object must be descendant of TYPE_DEVICE. So QOM TYPE_CPU should be inherited
      from TYPE_DEVICE at least or TYPE_SYS_BUS_DEVICE.
   2. hot-plug on the bus should be allowed. if we ditch icc bus then we should allow
      hot-plug on sysbus. Can we do this? (i.e. it seems that for P4 and later cpus
      sysbus should be hot-plugable).
   3. should DeviceClass.init be used for cpu initialization or should .instance_init
      do all the job and make DeviceClass.init nop?

Another approach that tries to re-use device_add interface:
   1. allow run-time type detection in qdev_device_add and execute separate branch for
      TYPE_CPU. This way we could easily use links<> on sysbus
   2. device_del will require the same hacking as device_add
   3. apic now is sysbus device, question is what will be lost if it is attached to link and
      won't be sysbus_device_type anymore?
   4. will reset called on sysbus reach apic/cpu if it is on the link?

Any opinions on direction I should look more closely?

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Andreas Färber]

Am 24.02.2012 14:05, schrieb Igor Mammedov:
> On 02/17/2012 06:02 PM, Igor Mammedov wrote:
>> So I'd rather drop ICC patch and try your approach with CPU links<>, I
>> see no
>> point in introducing new bus providing we have an alternative model
>> and existing
>> bus for the task.
> 
> I've looked at device_add command and qdev_device_add it uses for doing
> actual work
> and in current state it requires (based on Andreas' qom_cpu branch):
> 
> For approach where apic and cpu hot-plugged to sysbus.
>   1. created object must be descendant of TYPE_DEVICE. So QOM TYPE_CPU
> should be inherited
>      from TYPE_DEVICE at least or TYPE_SYS_BUS_DEVICE.
>   2. hot-plug on the bus should be allowed. if we ditch icc bus then we
> should allow
>      hot-plug on sysbus. Can we do this? (i.e. it seems that for P4 and
> later cpus
>      sysbus should be hot-plugable).
>   3. should DeviceClass.init be used for cpu initialization or should
> .instance_init
>      do all the job and make DeviceClass.init nop?

SysBus is supposed to go away in Anthony's upcoming 4th QOM series, so
I'd rather not base a new series on that.

The issue with TYPE_DEVICE is that we don't want to leak that into the
user emulators (would break the build), and any infrastructure only
available to qdev should gradually be made accessible to all objects
(Paolo has done some work in that direction wrt properties). So the main
remaining difference between Object and Device is the GPIO IRQ support.
Anthony wanted to introduce Pin objects to replace qemu_irq.

As for init, the idea was to have initialization code in an initfn, the
resulting state is then supposed to be overridable by the user (e.g.,
set family, model, stepping differently on the CPU) and then do the
construction work in a realize function that corresponds more closely to
DeviceClass::init. This too, still needs to be generalized.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Paolo Bonzini]

On 02/24/2012 02:30 PM, Andreas Färber wrote:
> SysBus is supposed to go away in Anthony's upcoming 4th QOM series, so
> I'd rather not base a new series on that.

Not sure about that.  I haven't understood well the scope of the series,
but I think it only converted buses to QOM, it didn't kill them.
Perhaps SysBus was special, in which case it would become automatically
hotpluggable: just create a new QOM object.

> The issue with TYPE_DEVICE is that we don't want to leak that into the
> user emulators (would break the build), and any infrastructure only
> available to qdev should gradually be made accessible to all objects
> (Paolo has done some work in that direction wrt properties).

I haven't, but it would be next on the list of things to do.

> So the main remaining difference between Object and Device is the
> GPIO IRQ support. Anthony wanted to introduce Pin objects to replace
> qemu_irq.

Aiming at "replacing" is a bad idea unless you can do it fast and
painlessly.  Adding gpio_in and gpio_out property types would be more
useful and would let you expose qemu_irq as QOM.  You can then change
the existing qdev.c functions to operate on those new property types.

Paolo

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Anthony Liguori]

On 02/24/2012 07:40 AM, Paolo Bonzini wrote:
> On 02/24/2012 02:30 PM, Andreas Färber wrote:
>> SysBus is supposed to go away in Anthony's upcoming 4th QOM series, so
>> I'd rather not base a new series on that.
>
> Not sure about that.  I haven't understood well the scope of the series,
> but I think it only converted buses to QOM, it didn't kill them.
> Perhaps SysBus was special, in which case it would become automatically
> hotpluggable: just create a new QOM object.
>
>> The issue with TYPE_DEVICE is that we don't want to leak that into the
>> user emulators (would break the build), and any infrastructure only
>> available to qdev should gradually be made accessible to all objects
>> (Paolo has done some work in that direction wrt properties).
>
> I haven't, but it would be next on the list of things to do.
>
>> So the main remaining difference between Object and Device is the
>> GPIO IRQ support. Anthony wanted to introduce Pin objects to replace
>> qemu_irq.
>
> Aiming at "replacing" is a bad idea unless you can do it fast and
> painlessly.  Adding gpio_in and gpio_out property types would be more
> useful and would let you expose qemu_irq as QOM.

I agree with you in principle, but in practice, there is not obvious way to 
serialize gpio_in/gpio_out via Visitors.  Finding some way to do it as an 
integer is clearly wrong IMHO.

I think a simple Pin object with the following interfaces:

/**
  * Connect a pin to a qemu_irq such that whenever the pin is
  * raised, qemu_irq_raise() is called too on irq.
  */
void pin_connect_qemu_irq(Pin *obj, qemu_irq irq);

/**
  * Returns a qemu_irq such that whenever qemu_irq_raise() is
  * called, pin_set_level(obj, true) is called.
  */
qemu_irq pin_get_qemu_irq(Pin *obj);

Let's you incrementally refactor objects to use Pins while maintaining the 
existing qemu_irq infrastructure.

Regards,

Anthony Liguori

>  You can then change
> the existing qdev.c functions to operate on those new property types.
>
> Paolo

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Paolo Bonzini]

On 02/24/2012 02:47 PM, Anthony Liguori wrote:
>>
> 
> I agree with you in principle, but in practice, there is not obvious way
> to serialize gpio_in/gpio_out via Visitors.  Finding some way to do it
> as an integer is clearly wrong IMHO.

"%s/gpio_in[%d]" % (object_get_canonical_path(...), opaque->n) is what I
was thinking about.

> I think a simple Pin object with the following interfaces:
> 
> /**
>  * Connect a pin to a qemu_irq such that whenever the pin is
>  * raised, qemu_irq_raise() is called too on irq.
>  */
> void pin_connect_qemu_irq(Pin *obj, qemu_irq irq);
> 
> /**
>  * Returns a qemu_irq such that whenever qemu_irq_raise() is
>  * called, pin_set_level(obj, true) is called.
>  */
> qemu_irq pin_get_qemu_irq(Pin *obj);
> 
> Let's you incrementally refactor objects to use Pins while maintaining the existing qemu_irq infrastructure.

Sure, a simple bridge is a fine alternative.  What I'm not sure about is
making Pins stateful, because that means you have to serialize them.

Paolo

Re: [Qemu-devel] [PATCH 1/7] Introduce a new bus "ICC" to connect APIC

[Anthony Liguori]

On 02/24/2012 07:50 AM, Paolo Bonzini wrote:
> On 02/24/2012 02:47 PM, Anthony Liguori wrote:
>>>
>>
>> I agree with you in principle, but in practice, there is not obvious way
>> to serialize gpio_in/gpio_out via Visitors.  Finding some way to do it
>> as an integer is clearly wrong IMHO.
>
> "%s/gpio_in[%d]" % (object_get_canonical_path(...), opaque->n) is what I
> was thinking about.

This creates another namespace that's independent of the QOM graph.  This is 
something we should try to avoid.

>> I think a simple Pin object with the following interfaces:
>>
>> /**
>>   * Connect a pin to a qemu_irq such that whenever the pin is
>>   * raised, qemu_irq_raise() is called too on irq.
>>   */
>> void pin_connect_qemu_irq(Pin *obj, qemu_irq irq);
>>
>> /**
>>   * Returns a qemu_irq such that whenever qemu_irq_raise() is
>>   * called, pin_set_level(obj, true) is called.
>>   */
>> qemu_irq pin_get_qemu_irq(Pin *obj);
>>
>> Let's you incrementally refactor objects to use Pins while maintaining the existing qemu_irq infrastructure.
>
> Sure, a simple bridge is a fine alternative.  What I'm not sure about is
> making Pins stateful, because that means you have to serialize them.

Being stateful is a feature but the concept would work just as well if you 
didn't store the pin state.  Then it just looks like:

struct Pin
{
    Object parent;

    /*< private >*/
    NotifierLister level_change_notifiers;
};

The reason to introduce another type (instead of attempting to convert qemu_irq) 
is that the life cycle of qemu_irq is very un-QOM.  I don't think we can do it 
without incrementally refactoring the users of qemu_irq and a new type makes it 
easier to do that incrementally.

Regards,

Anthony Liguori

> Paolo

[Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Igor Mammedov]

Convert pc cpu to qdev device that is attached to icc bus, later
hot-plug ability of icc bus will allow to implement cpu hot-plug.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c              |   62 +++++++++++++++++++++++++++++++++++++++++++------
 target-i386/cpu.h    |    1 +
 target-i386/helper.c |   26 ++++++++++++++------
 3 files changed, 73 insertions(+), 16 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 33d8090..b8db5dc 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
+typedef struct CPUPC {
+    ICCBusDevice busdev;
+    char *model;
+    CPUState state;
+} CPUPC;
+
 static void pc_cpu_reset(void *opaque)
 {
     CPUState *env = opaque;
@@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque)
     env->halted = !cpu_is_bsp(env);
 }
 
-static CPUState *pc_new_cpu(const char *cpu_model)
+static DeviceState *pc_new_cpu(const char *cpu_model)
 {
-    CPUState *env;
+    DeviceState *dev;
+    BusState *b;
 
-    env = cpu_init(cpu_model);
-    if (!env) {
-        fprintf(stderr, "Unable to find x86 CPU definition\n");
-        exit(1);
+    b = get_icc_bus();
+    dev = qdev_create(b, "cpu-pc");
+    if (!dev) {
+        return NULL;
+    }
+    qdev_prop_set_string(dev, "model", g_strdup(cpu_model));
+    if (qdev_init(dev) < 0) {
+        return NULL;
+    }
+    return dev;
+}
+
+static int cpu_device_init(ICCBusDevice *dev)
+{
+    CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
+    CPUState *env = &cpu->state;
+
+    if (cpu_x86_init_inplace(env, cpu->model) < 0) {
+        return -1;
     }
+
     if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
         env->apic_state = apic_init(env, env->cpuid_apic_id);
     }
-    qemu_register_reset(pc_cpu_reset, env);
+
+    return 0;
+}
+
+static void cpu_device_reset(DeviceState *dev) {
+    CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev);
+    CPUState *env = &cpu->state;
     pc_cpu_reset(env);
-    return env;
 }
 
+static ICCBusDeviceInfo cpu_device_info = {
+    .qdev.name = "cpu-pc",
+    .qdev.size = sizeof(CPUPC),
+    .qdev.reset = cpu_device_reset,
+    .init = cpu_device_init,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_STRING("model", CPUPC, model),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void pc_register_devices(void)
+{
+    iccbus_register_devinfo(&cpu_device_info);
+}
+
+device_init(pc_register_devices);
+
 void pc_cpus_init(const char *cpu_model)
 {
     int i;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 7e66bcf..830b65e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -775,6 +775,7 @@ typedef struct CPUX86State {
 } CPUX86State;
 
 CPUX86State *cpu_x86_init(const char *cpu_model);
+int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
 void cpu_x86_close(CPUX86State *s);
 void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 2586aff..df2f5ba 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
     return 1;
 }
 
-CPUX86State *cpu_x86_init(const char *cpu_model)
+int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model)
 {
-    CPUX86State *env;
     static int inited;
 
-    env = g_malloc0(sizeof(CPUX86State));
+    if (cpu_x86_register(env, cpu_model) < 0) {
+        fprintf(stderr, "Unable to find x86 CPU definition\n");
+        return -1;
+    }
     cpu_exec_init(env);
     env->cpu_model_str = cpu_model;
 
@@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
             cpu_set_debug_excp_handler(breakpoint_handler);
 #endif
     }
-    if (cpu_x86_register(env, cpu_model) < 0) {
-        cpu_x86_close(env);
-        return NULL;
-    }
     env->cpuid_apic_id = env->cpu_index;
     mce_init(env);
 
     qemu_init_vcpu(env);
 
-    return env;
+    return 0;
 }
 
+CPUX86State *cpu_x86_init(const char *cpu_model)
+{
+    CPUX86State *env;
+
+    env = g_malloc0(sizeof(CPUX86State));
+    if (cpu_x86_init_inplace(env, cpu_model) < 0) {
+        g_free(env);
+        return NULL;
+    }
+
+    return env;
+}
 #if !defined(CONFIG_USER_ONLY)
 void do_cpu_init(CPUState *env)
 {
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Jan Kiszka]

On 2012-02-16 00:16, Igor Mammedov wrote:
> Convert pc cpu to qdev device that is attached to icc bus, later
> hot-plug ability of icc bus will allow to implement cpu hot-plug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/pc.c              |   62 +++++++++++++++++++++++++++++++++++++++++++------
>  target-i386/cpu.h    |    1 +
>  target-i386/helper.c |   26 ++++++++++++++------
>  3 files changed, 73 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 33d8090..b8db5dc 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>      }
>  }
>  
> +typedef struct CPUPC {
> +    ICCBusDevice busdev;
> +    char *model;
> +    CPUState state;
> +} CPUPC;
> +
>  static void pc_cpu_reset(void *opaque)
>  {
>      CPUState *env = opaque;
> @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque)
>      env->halted = !cpu_is_bsp(env);
>  }
>  
> -static CPUState *pc_new_cpu(const char *cpu_model)
> +static DeviceState *pc_new_cpu(const char *cpu_model)
>  {
> -    CPUState *env;
> +    DeviceState *dev;
> +    BusState *b;
>  
> -    env = cpu_init(cpu_model);
> -    if (!env) {
> -        fprintf(stderr, "Unable to find x86 CPU definition\n");
> -        exit(1);
> +    b = get_icc_bus();
> +    dev = qdev_create(b, "cpu-pc");
> +    if (!dev) {
> +        return NULL;
> +    }
> +    qdev_prop_set_string(dev, "model", g_strdup(cpu_model));
> +    if (qdev_init(dev) < 0) {
> +        return NULL;
> +    }
> +    return dev;
> +}
> +
> +static int cpu_device_init(ICCBusDevice *dev)
> +{
> +    CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
> +    CPUState *env = &cpu->state;
> +
> +    if (cpu_x86_init_inplace(env, cpu->model) < 0) {
> +        return -1;
>      }
> +
>      if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>          env->apic_state = apic_init(env, env->cpuid_apic_id);
>      }
> -    qemu_register_reset(pc_cpu_reset, env);
> +
> +    return 0;
> +}
> +
> +static void cpu_device_reset(DeviceState *dev) {
> +    CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev);
> +    CPUState *env = &cpu->state;
>      pc_cpu_reset(env);
> -    return env;
>  }
>  
> +static ICCBusDeviceInfo cpu_device_info = {
> +    .qdev.name = "cpu-pc",
> +    .qdev.size = sizeof(CPUPC),
> +    .qdev.reset = cpu_device_reset,
> +    .init = cpu_device_init,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_STRING("model", CPUPC, model),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void pc_register_devices(void)
> +{
> +    iccbus_register_devinfo(&cpu_device_info);
> +}
> +
> +device_init(pc_register_devices);
> +
>  void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 7e66bcf..830b65e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -775,6 +775,7 @@ typedef struct CPUX86State {
>  } CPUX86State;
>  
>  CPUX86State *cpu_x86_init(const char *cpu_model);
> +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model);
>  int cpu_x86_exec(CPUX86State *s);
>  void cpu_x86_close(CPUX86State *s);
>  void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg);
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 2586aff..df2f5ba 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
>      return 1;
>  }
>  
> -CPUX86State *cpu_x86_init(const char *cpu_model)
> +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model)
>  {
> -    CPUX86State *env;
>      static int inited;
>  
> -    env = g_malloc0(sizeof(CPUX86State));
> +    if (cpu_x86_register(env, cpu_model) < 0) {
> +        fprintf(stderr, "Unable to find x86 CPU definition\n");
> +        return -1;
> +    }
>      cpu_exec_init(env);
>      env->cpu_model_str = cpu_model;
>  
> @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>              cpu_set_debug_excp_handler(breakpoint_handler);
>  #endif
>      }
> -    if (cpu_x86_register(env, cpu_model) < 0) {
> -        cpu_x86_close(env);
> -        return NULL;
> -    }
>      env->cpuid_apic_id = env->cpu_index;
>      mce_init(env);
>  
>      qemu_init_vcpu(env);
>  
> -    return env;
> +    return 0;
>  }
>  
> +CPUX86State *cpu_x86_init(const char *cpu_model)
> +{
> +    CPUX86State *env;
> +
> +    env = g_malloc0(sizeof(CPUX86State));
> +    if (cpu_x86_init_inplace(env, cpu_model) < 0) {
> +        g_free(env);
> +        return NULL;
> +    }
> +
> +    return env;
> +}
>  #if !defined(CONFIG_USER_ONLY)
>  void do_cpu_init(CPUState *env)
>  {

QOM is required here as well.

Andreas is also working on CPU devices. I guess you should coordinate
efforts.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Jan Kiszka]

On 2012-02-16 00:16, Igor Mammedov wrote:
> +static ICCBusDeviceInfo cpu_device_info = {
> +    .qdev.name = "cpu-pc",
> +    .qdev.size = sizeof(CPUPC),
> +    .qdev.reset = cpu_device_reset,
> +    .init = cpu_device_init,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_STRING("model", CPUPC, model),

And how do you pass in feature flags? Or the core layout? Basically both
-cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx"
(not "pc") commands. As a shortcut, "-smp N" would continue to replicate
a "-device cpu-XX" N times. Otherwise, configuration becomes inconsistent.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Anthony Liguori]

On 02/16/2012 06:01 AM, Jan Kiszka wrote:
> On 2012-02-16 00:16, Igor Mammedov wrote:
>> +static ICCBusDeviceInfo cpu_device_info = {
>> +    .qdev.name = "cpu-pc",
>> +    .qdev.size = sizeof(CPUPC),
>> +    .qdev.reset = cpu_device_reset,
>> +    .init = cpu_device_init,
>> +    .qdev.props = (Property[]) {
>> +        DEFINE_PROP_STRING("model", CPUPC, model),
>
> And how do you pass in feature flags? Or the core layout? Basically both
> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx"
> (not "pc") commands.

The approach that I'd recommend is:

1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace 
all references to members of CPU_COMMON_STATE appropriately.

2) convert as many users of CPUState to CPUCommonState as possible.

3) make CPUCommonState a base class, move it to the front of the structure, and 
attempt to measure the impact to TCG.

4) if (3) is significant, special case things in QOM

5) make target specific code use target specific CPUState typename

6) eliminate #define CPUState

7) make on-processor devices (like lapic) children of target specific CPUState

8) express target specific flags/features via QOM properties

9) make machine expose target specific CPUState links that can be set by the user.

Regards,

Anthony Liguori

> As a shortcut, "-smp N" would continue to replicate
> a "-device cpu-XX" N times. Otherwise, configuration becomes inconsistent.
>
> Jan
>

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Jan Kiszka]

On 2012-02-16 13:51, Anthony Liguori wrote:
> On 02/16/2012 06:01 AM, Jan Kiszka wrote:
>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>> +static ICCBusDeviceInfo cpu_device_info = {
>>> +    .qdev.name = "cpu-pc",
>>> +    .qdev.size = sizeof(CPUPC),
>>> +    .qdev.reset = cpu_device_reset,
>>> +    .init = cpu_device_init,
>>> +    .qdev.props = (Property[]) {
>>> +        DEFINE_PROP_STRING("model", CPUPC, model),
>>
>> And how do you pass in feature flags? Or the core layout? Basically both
>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx"
>> (not "pc") commands.
> 
> The approach that I'd recommend is:
> 
> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace 
> all references to members of CPU_COMMON_STATE appropriately.
> 
> 2) convert as many users of CPUState to CPUCommonState as possible.
> 
> 3) make CPUCommonState a base class, move it to the front of the structure, and 
> attempt to measure the impact to TCG.
> 
> 4) if (3) is significant, special case things in QOM
> 
> 5) make target specific code use target specific CPUState typename
> 
> 6) eliminate #define CPUState
> 
> 7) make on-processor devices (like lapic) children of target specific CPUState
> 
> 8) express target specific flags/features via QOM properties
> 
> 9) make machine expose target specific CPUState links that can be set by the user.

Also, I'm wondering what names those CPUs should have. I think we should
expose model names as device names and avoid a "model" property.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Anthony Liguori]

On 02/16/2012 06:54 AM, Jan Kiszka wrote:
> On 2012-02-16 13:51, Anthony Liguori wrote:
>> On 02/16/2012 06:01 AM, Jan Kiszka wrote:
>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>> +static ICCBusDeviceInfo cpu_device_info = {
>>>> +    .qdev.name = "cpu-pc",
>>>> +    .qdev.size = sizeof(CPUPC),
>>>> +    .qdev.reset = cpu_device_reset,
>>>> +    .init = cpu_device_init,
>>>> +    .qdev.props = (Property[]) {
>>>> +        DEFINE_PROP_STRING("model", CPUPC, model),
>>>
>>> And how do you pass in feature flags? Or the core layout? Basically both
>>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx"
>>> (not "pc") commands.
>>
>> The approach that I'd recommend is:
>>
>> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace
>> all references to members of CPU_COMMON_STATE appropriately.
>>
>> 2) convert as many users of CPUState to CPUCommonState as possible.
>>
>> 3) make CPUCommonState a base class, move it to the front of the structure, and
>> attempt to measure the impact to TCG.
>>
>> 4) if (3) is significant, special case things in QOM
>>
>> 5) make target specific code use target specific CPUState typename
>>
>> 6) eliminate #define CPUState
>>
>> 7) make on-processor devices (like lapic) children of target specific CPUState
>>
>> 8) express target specific flags/features via QOM properties
>>
>> 9) make machine expose target specific CPUState links that can be set by the user.
>
> Also, I'm wondering what names those CPUs should have. I think we should
> expose model names as device names and avoid a "model" property.

Yeah, I think we want a CPUX86State and then a bunch of subclasses defined by that.

By making use of the class_data field, we can register classes based on 
combinations of feature flags.  So for instance:

static TypeInfo opteron_g2 = {
   .name = "cpu-opteron-g2",
   .parent = TYPE_CPU_X86",
   .class_init = cpu_x86_generic_class_init,
   .class_data = (CPUX86Definition[]){
       .level = 5,
       .vendor = "AuthenticAMD",
       .family = 15,
       .model = 6,
       .stepping = 1,
       .feature_edx = "sse2 sse fxsr mmx pat cmov pge sep apic cx8 mce pae msr 
tsc pse de fpu   mtrr clflush mca pse36",
       ...
    },
};

And obviously, we can still using the existing cpudef directive to generate types.

Regards,

Anthony Liguori

>
> Jan
>

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Lai Jiangshan]

On 02/16/2012 08:51 PM, Anthony Liguori wrote:
> On 02/16/2012 06:01 AM, Jan Kiszka wrote:
>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>> +static ICCBusDeviceInfo cpu_device_info = {
>>> +    .qdev.name = "cpu-pc",
>>> +    .qdev.size = sizeof(CPUPC),
>>> +    .qdev.reset = cpu_device_reset,
>>> +    .init = cpu_device_init,
>>> +    .qdev.props = (Property[]) {
>>> +        DEFINE_PROP_STRING("model", CPUPC, model),
>>
>> And how do you pass in feature flags? Or the core layout? Basically both
>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx"
>> (not "pc") commands.
> 
> The approach that I'd recommend is:
> 
> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately.
> 

Hi, All

I just tried this for several days, it will result a huge patch,
it is hard for human, any suggestion?
(I used Semantic patches script: http://coccinelle.lip6.fr/, it is still hard,
it still leaves huge part which needs manual conversion/fix)

I will take part in implementing cpu-hotplug future for qemu,
Could you give me some tips/suggestions? What approach should I take?
The main work of my "taking part in" is assisting for this community,
So you can assign some works/steps to me.

Thanks,
Lai

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Andreas Färber]

Am 13.03.2012 10:32, schrieb Lai Jiangshan:
> On 02/16/2012 08:51 PM, Anthony Liguori wrote:
>> On 02/16/2012 06:01 AM, Jan Kiszka wrote:
>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>> +static ICCBusDeviceInfo cpu_device_info = {
>>>> +    .qdev.name = "cpu-pc",
>>>> +    .qdev.size = sizeof(CPUPC),
>>>> +    .qdev.reset = cpu_device_reset,
>>>> +    .init = cpu_device_init,
>>>> +    .qdev.props = (Property[]) {
>>>> +        DEFINE_PROP_STRING("model", CPUPC, model),
>>>
>>> And how do you pass in feature flags? Or the core layout? Basically both
>>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx"
>>> (not "pc") commands.
>>
>> The approach that I'd recommend is:
>>
>> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately.
> 
> I just tried this for several days, it will result a huge patch,
> it is hard for human, any suggestion?
> (I used Semantic patches script: http://coccinelle.lip6.fr/, it is still hard,
> it still leaves huge part which needs manual conversion/fix)
> 
> I will take part in implementing cpu-hotplug future for qemu,
> Could you give me some tips/suggestions? What approach should I take?

In short: Don't!

Please instead provide feedback on my series which already does
something like that for you, available since multiple weeks. If a
different naming is preferred, I can change it.

The approach I have taken is:
1. CPUState -> CPU{X86,...}State, CPUArchState as alias where necessary
2. Embed CPU$archState in $archCPU
3. Step by step move CPU_COMMON_STATE from CPUArchState into CPUState

Steps 1+2 are done, 2 is partially on the list (e.g., [1]), the rest on
my qom-cpu-wip branch [2].
Step 3 is shown for icount on qom-cpu-wip as well as for multiple
target-specific fields such as target-arm features [3]. Contributions
welcome.

Whatever naming and order we decide on, this WILL touch a lot of code.
If you start doing this in some random different way we'll end up with
conflicting series on the list, wasting each other's time.

Thanks,
Andreas

[1] http://patchwork.ozlabs.org/patch/145800/
[2] http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-cpu-wip
[3] http://patchwork.ozlabs.org/patch/145874/

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Lai Jiangshan]

On 03/13/2012 07:04 PM, Andreas Färber wrote:
> Am 13.03.2012 10:32, schrieb Lai Jiangshan:
>> On 02/16/2012 08:51 PM, Anthony Liguori wrote:
>>> On 02/16/2012 06:01 AM, Jan Kiszka wrote:
>>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>>> +static ICCBusDeviceInfo cpu_device_info = {
>>>>> +    .qdev.name = "cpu-pc",
>>>>> +    .qdev.size = sizeof(CPUPC),
>>>>> +    .qdev.reset = cpu_device_reset,
>>>>> +    .init = cpu_device_init,
>>>>> +    .qdev.props = (Property[]) {
>>>>> +        DEFINE_PROP_STRING("model", CPUPC, model),
>>>>
>>>> And how do you pass in feature flags? Or the core layout? Basically both
>>>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx"
>>>> (not "pc") commands.
>>>
>>> The approach that I'd recommend is:
>>>
>>> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately.
>>
>> I just tried this for several days, it will result a huge patch,
>> it is hard for human, any suggestion?
>> (I used Semantic patches script: http://coccinelle.lip6.fr/, it is still hard,
>> it still leaves huge part which needs manual conversion/fix)
>>
>> I will take part in implementing cpu-hotplug future for qemu,
>> Could you give me some tips/suggestions? What approach should I take?
> 
> In short: Don't!
> 
> Please instead provide feedback on my series which already does
> something like that for you, available since multiple weeks. If a
> different naming is preferred, I can change it.
> 
> The approach I have taken is:
> 1. CPUState -> CPU{X86,...}State, CPUArchState as alias where necessary
> 2. Embed CPU$archState in $archCPU
> 3. Step by step move CPU_COMMON_STATE from CPUArchState into CPUState
> 
> Steps 1+2 are done, 2 is partially on the list (e.g., [1]), the rest on
> my qom-cpu-wip branch [2].
> Step 3 is shown for icount on qom-cpu-wip as well as for multiple
> target-specific fields such as target-arm features [3]. Contributions
> welcome.


OK, your work can't be done automatically, but it do be worth when
there are multiple target-specific fields existed.

Anthony Liguori, Jan Kiszka,

Could you light me what can I do for cpu-hotplug.
I saw several approaches are sent to the maillist, but they are
not accepted, so I don't know how to take part in.

Thanks,
Lai

> 
> Whatever naming and order we decide on, this WILL touch a lot of code.
> If you start doing this in some random different way we'll end up with
> conflicting series on the list, wasting each other's time.
> 
> Thanks,
> Andreas
> 
> [1] http://patchwork.ozlabs.org/patch/145800/
> [2] http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-cpu-wip
> [3] http://patchwork.ozlabs.org/patch/145874/
> 

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Igor Mammedov]

On 03/14/2012 08:59 AM, Lai Jiangshan wrote:
> On 03/13/2012 07:04 PM, Andreas Färber wrote:
>> Am 13.03.2012 10:32, schrieb Lai Jiangshan:
>>> On 02/16/2012 08:51 PM, Anthony Liguori wrote:
>>>> On 02/16/2012 06:01 AM, Jan Kiszka wrote:
>>>>> On 2012-02-16 00:16, Igor Mammedov wrote:
>>>>>> +static ICCBusDeviceInfo cpu_device_info = {
>>>>>> +    .qdev.name = "cpu-pc",
>>>>>> +    .qdev.size = sizeof(CPUPC),
>>>>>> +    .qdev.reset = cpu_device_reset,
>>>>>> +    .init = cpu_device_init,
>>>>>> +    .qdev.props = (Property[]) {
>>>>>> +        DEFINE_PROP_STRING("model", CPUPC, model),
>>>>>
>>>>> And how do you pass in feature flags? Or the core layout? Basically both
>>>>> -cpu and -smp need to be expressible via multiple "-device cpu-x86,xxx"
>>>>> (not "pc") commands.
>>>>
>>>> The approach that I'd recommend is:
>>>>
>>>> 1) convert CPU_COMMON_STATE to a structure named CPUCommonState, query/replace all references to members of CPU_COMMON_STATE appropriately.
>>>
>>> I just tried this for several days, it will result a huge patch,
>>> it is hard for human, any suggestion?
>>> (I used Semantic patches script: http://coccinelle.lip6.fr/, it is still hard,
>>> it still leaves huge part which needs manual conversion/fix)
>>>
>>> I will take part in implementing cpu-hotplug future for qemu,
>>> Could you give me some tips/suggestions? What approach should I take?
>>
>> In short: Don't!
>>
>> Please instead provide feedback on my series which already does
>> something like that for you, available since multiple weeks. If a
>> different naming is preferred, I can change it.
>>
>> The approach I have taken is:
>> 1. CPUState ->  CPU{X86,...}State, CPUArchState as alias where necessary
>> 2. Embed CPU$archState in $archCPU
>> 3. Step by step move CPU_COMMON_STATE from CPUArchState into CPUState
>>
>> Steps 1+2 are done, 2 is partially on the list (e.g., [1]), the rest on
>> my qom-cpu-wip branch [2].
>> Step 3 is shown for icount on qom-cpu-wip as well as for multiple
>> target-specific fields such as target-arm features [3]. Contributions
>> welcome.
>
>
> OK, your work can't be done automatically, but it do be worth when
> there are multiple target-specific fields existed.
>
> Anthony Liguori, Jan Kiszka,
>
> Could you light me what can I do for cpu-hotplug.
> I saw several approaches are sent to the maillist, but they are
> not accepted, so I don't know how to take part in.

As I see it, there is not much to do from cpu hot-plug perspective.
It's just a matter of adaptation QOM-ified cpus for usage from
qdev device_add, and I'm working on it.
However, there is a lot to be done in cpu unplug area:
   - host side: there is unaccepted patches to destroy vcpu
     during VM-lifecycle. They are still need to be worked on:
      "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm"
   - linux guest side: kernel can receive ACPI request to unplug cpu,
     but does nothing with it (last time I've tested it with 3.2 kernel),
     You might wish to look at following mail threads:
       https://lkml.org/lkml/2011/9/30/18
       http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html

-- 
-----
  Igor

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Vasilis Liaskovitis]

Hi,

On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote:
> On 03/14/2012 08:59 AM, Lai Jiangshan wrote:
> >not accepted, so I don't know how to take part in.
> 
> As I see it, there is not much to do from cpu hot-plug perspective.
> It's just a matter of adaptation QOM-ified cpus for usage from
> qdev device_add, and I'm working on it.
> However, there is a lot to be done in cpu unplug area:
>   - host side: there is unaccepted patches to destroy vcpu
>     during VM-lifecycle. They are still need to be worked on:
>      "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm"
>   - linux guest side: kernel can receive ACPI request to unplug cpu,
>     but does nothing with it (last time I've tested it with 3.2 kernel),
>     You might wish to look at following mail threads:
>       https://lkml.org/lkml/2011/9/30/18
>       http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html

I also plan to resubmit the qemu-side of ACPI cpu unplug request:
http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html
so that they work independently of the "host side" patches mentioned above. 

It would be great for the QOMify/hotplug/icc patches to be accepted soon,
since this would make unplug testing/development more straightoward.

thanks,

- Vasilis

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Gleb Natapov]

On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote:
> Hi,
> 
> On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote:
> > On 03/14/2012 08:59 AM, Lai Jiangshan wrote:
> > >not accepted, so I don't know how to take part in.
> > 
> > As I see it, there is not much to do from cpu hot-plug perspective.
> > It's just a matter of adaptation QOM-ified cpus for usage from
> > qdev device_add, and I'm working on it.
> > However, there is a lot to be done in cpu unplug area:
> >   - host side: there is unaccepted patches to destroy vcpu
> >     during VM-lifecycle. They are still need to be worked on:
> >      "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm"
> >   - linux guest side: kernel can receive ACPI request to unplug cpu,
> >     but does nothing with it (last time I've tested it with 3.2 kernel),
> >     You might wish to look at following mail threads:
> >       https://lkml.org/lkml/2011/9/30/18
> >       http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html
> 
> I also plan to resubmit the qemu-side of ACPI cpu unplug request:
> http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html
> so that they work independently of the "host side" patches mentioned above. 
> 
> It would be great for the QOMify/hotplug/icc patches to be accepted soon,
> since this would make unplug testing/development more straightoward.
> 
On a different note, are your going to continue working on your memory hot plug series?
I am going to look at it now.

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Anthony Liguori]

On 03/14/2012 10:23 AM, Gleb Natapov wrote:
> On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote:
>> Hi,
>>
>> On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote:
>>> On 03/14/2012 08:59 AM, Lai Jiangshan wrote:
>>>> not accepted, so I don't know how to take part in.
>>>
>>> As I see it, there is not much to do from cpu hot-plug perspective.
>>> It's just a matter of adaptation QOM-ified cpus for usage from
>>> qdev device_add, and I'm working on it.
>>> However, there is a lot to be done in cpu unplug area:
>>>    - host side: there is unaccepted patches to destroy vcpu
>>>      during VM-lifecycle. They are still need to be worked on:
>>>       "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm"
>>>    - linux guest side: kernel can receive ACPI request to unplug cpu,
>>>      but does nothing with it (last time I've tested it with 3.2 kernel),
>>>      You might wish to look at following mail threads:
>>>        https://lkml.org/lkml/2011/9/30/18
>>>        http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html
>>
>> I also plan to resubmit the qemu-side of ACPI cpu unplug request:
>> http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html
>> so that they work independently of the "host side" patches mentioned above.
>>
>> It would be great for the QOMify/hotplug/icc patches to be accepted soon,
>> since this would make unplug testing/development more straightoward.
>>
> On a different note, are your going to continue working on your memory hot plug series?
> I am going to look at it now.

Memory hotplug..  that's going to be fun :-)

Regards,

Anthony Liguori

>
> --
> 			Gleb.

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Gleb Natapov]

On Wed, Mar 14, 2012 at 10:32:37AM -0500, Anthony Liguori wrote:
> On 03/14/2012 10:23 AM, Gleb Natapov wrote:
> >On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote:
> >>Hi,
> >>
> >>On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote:
> >>>On 03/14/2012 08:59 AM, Lai Jiangshan wrote:
> >>>>not accepted, so I don't know how to take part in.
> >>>
> >>>As I see it, there is not much to do from cpu hot-plug perspective.
> >>>It's just a matter of adaptation QOM-ified cpus for usage from
> >>>qdev device_add, and I'm working on it.
> >>>However, there is a lot to be done in cpu unplug area:
> >>>   - host side: there is unaccepted patches to destroy vcpu
> >>>     during VM-lifecycle. They are still need to be worked on:
> >>>      "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm"
> >>>   - linux guest side: kernel can receive ACPI request to unplug cpu,
> >>>     but does nothing with it (last time I've tested it with 3.2 kernel),
> >>>     You might wish to look at following mail threads:
> >>>       https://lkml.org/lkml/2011/9/30/18
> >>>       http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html
> >>
> >>I also plan to resubmit the qemu-side of ACPI cpu unplug request:
> >>http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html
> >>so that they work independently of the "host side" patches mentioned above.
> >>
> >>It would be great for the QOMify/hotplug/icc patches to be accepted soon,
> >>since this would make unplug testing/development more straightoward.
> >>
> >On a different note, are your going to continue working on your memory hot plug series?
> >I am going to look at it now.
> 
> Memory hotplug..  that's going to be fun :-)
> 
Why? What fundamental difficultly do you anticipate?

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Anthony Liguori]

On 03/14/2012 10:35 AM, Gleb Natapov wrote:
> On Wed, Mar 14, 2012 at 10:32:37AM -0500, Anthony Liguori wrote:
>> On 03/14/2012 10:23 AM, Gleb Natapov wrote:
>>> On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote:
>>>> Hi,
>>>>
>>>> On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote:
>>>>> On 03/14/2012 08:59 AM, Lai Jiangshan wrote:
>>>>>> not accepted, so I don't know how to take part in.
>>>>>
>>>>> As I see it, there is not much to do from cpu hot-plug perspective.
>>>>> It's just a matter of adaptation QOM-ified cpus for usage from
>>>>> qdev device_add, and I'm working on it.
>>>>> However, there is a lot to be done in cpu unplug area:
>>>>>    - host side: there is unaccepted patches to destroy vcpu
>>>>>      during VM-lifecycle. They are still need to be worked on:
>>>>>       "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm"
>>>>>    - linux guest side: kernel can receive ACPI request to unplug cpu,
>>>>>      but does nothing with it (last time I've tested it with 3.2 kernel),
>>>>>      You might wish to look at following mail threads:
>>>>>        https://lkml.org/lkml/2011/9/30/18
>>>>>        http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html
>>>>
>>>> I also plan to resubmit the qemu-side of ACPI cpu unplug request:
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html
>>>> so that they work independently of the "host side" patches mentioned above.
>>>>
>>>> It would be great for the QOMify/hotplug/icc patches to be accepted soon,
>>>> since this would make unplug testing/development more straightoward.
>>>>
>>> On a different note, are your going to continue working on your memory hot plug series?
>>> I am going to look at it now.
>>
>> Memory hotplug..  that's going to be fun :-)
>>
> Why? What fundamental difficultly do you anticipate?

There's still a few places in QEMU that assume that qemu_ram_get_ptr() returns a 
pointer that's good indefinitely.

We also don't have a mechanism to revoke cpu_physical_map() pointers.  Maybe the 
answer is reference counting and relying on being able to eventually release the 
memory...   Of course, then an unplug followed by an immediate plug would be 
complicated.

Hot add should be easy, hot remove will be pretty hard I think.

Regards,

Anthony Liguori

>
> --
> 			Gleb.

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Gleb Natapov]

On Wed, Mar 14, 2012 at 10:42:50AM -0500, Anthony Liguori wrote:
> On 03/14/2012 10:35 AM, Gleb Natapov wrote:
> >On Wed, Mar 14, 2012 at 10:32:37AM -0500, Anthony Liguori wrote:
> >>On 03/14/2012 10:23 AM, Gleb Natapov wrote:
> >>>On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote:
> >>>>Hi,
> >>>>
> >>>>On Wed, Mar 14, 2012 at 09:37:18AM +0100, Igor Mammedov wrote:
> >>>>>On 03/14/2012 08:59 AM, Lai Jiangshan wrote:
> >>>>>>not accepted, so I don't know how to take part in.
> >>>>>
> >>>>>As I see it, there is not much to do from cpu hot-plug perspective.
> >>>>>It's just a matter of adaptation QOM-ified cpus for usage from
> >>>>>qdev device_add, and I'm working on it.
> >>>>>However, there is a lot to be done in cpu unplug area:
> >>>>>   - host side: there is unaccepted patches to destroy vcpu
> >>>>>     during VM-lifecycle. They are still need to be worked on:
> >>>>>      "[Qemu-devel] [PATCH 0] A series patches for kvm&qemu to enable vcpu destruction in kvm"
> >>>>>   - linux guest side: kernel can receive ACPI request to unplug cpu,
> >>>>>     but does nothing with it (last time I've tested it with 3.2 kernel),
> >>>>>     You might wish to look at following mail threads:
> >>>>>       https://lkml.org/lkml/2011/9/30/18
> >>>>>       http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02254.html
> >>>>
> >>>>I also plan to resubmit the qemu-side of ACPI cpu unplug request:
> >>>>http://lists.nongnu.org/archive/html/qemu-devel/2012-01/msg03037.html
> >>>>so that they work independently of the "host side" patches mentioned above.
> >>>>
> >>>>It would be great for the QOMify/hotplug/icc patches to be accepted soon,
> >>>>since this would make unplug testing/development more straightoward.
> >>>>
> >>>On a different note, are your going to continue working on your memory hot plug series?
> >>>I am going to look at it now.
> >>
> >>Memory hotplug..  that's going to be fun :-)
> >>
> >Why? What fundamental difficultly do you anticipate?
> 
> There's still a few places in QEMU that assume that
> qemu_ram_get_ptr() returns a pointer that's good indefinitely.
> 
> We also don't have a mechanism to revoke cpu_physical_map()
> pointers.  Maybe the answer is reference counting and relying on
> being able to eventually release the memory...   Of course, then an
> unplug followed by an immediate plug would be complicated.
> 
Hmm, Avi assured me that with the memory API rework it should be easy :(
grep founds nothing about qemu_ram_get_ptr() and cpu_physical_map()
though. What should I look for?

> Hot add should be easy, hot remove will be pretty hard I think.
> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >--
> >			Gleb.

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Anthony Liguori]

On 03/14/2012 10:54 AM, Gleb Natapov wrote:
> On Wed, Mar 14, 2012 at 10:42:50AM -0500, Anthony Liguori wrote:
>>
>> There's still a few places in QEMU that assume that
>> qemu_ram_get_ptr() returns a pointer that's good indefinitely.
>>
>> We also don't have a mechanism to revoke cpu_physical_map()
>> pointers.  Maybe the answer is reference counting and relying on
>> being able to eventually release the memory...   Of course, then an
>> unplug followed by an immediate plug would be complicated.
>>
> Hmm, Avi assured me that with the memory API rework it should be easy :(
> grep founds nothing about qemu_ram_get_ptr() and cpu_physical_map()
> though. What should I look for?

memory_region_get_ptr() and cpu_memory_physical_map().

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Gleb Natapov]

On Wed, Mar 14, 2012 at 10:57:15AM -0500, Anthony Liguori wrote:
> On 03/14/2012 10:54 AM, Gleb Natapov wrote:
> >On Wed, Mar 14, 2012 at 10:42:50AM -0500, Anthony Liguori wrote:
> >>
> >>There's still a few places in QEMU that assume that
> >>qemu_ram_get_ptr() returns a pointer that's good indefinitely.
> >>
> >>We also don't have a mechanism to revoke cpu_physical_map()
> >>pointers.  Maybe the answer is reference counting and relying on
> >>being able to eventually release the memory...   Of course, then an
> >>unplug followed by an immediate plug would be complicated.
> >>
> >Hmm, Avi assured me that with the memory API rework it should be easy :(
> >grep founds nothing about qemu_ram_get_ptr() and cpu_physical_map()
> >though. What should I look for?
> 
> memory_region_get_ptr() and cpu_memory_physical_map().
> 
cpu_physical_memory_map() and qemu_get_ram_ptr() (called only via
memory_region_get_ram_ptr() by device model). According to
qemu_get_ram_ptr() comment it should be used only to access device
memory, not RAM (it is also used in softmmu code, but lets leave this
for now :)).

cpu_physical_memory_map() is used for DMA. When guest confirms memory
removal by calling _EJ() ACPI methods no DMA should be directed to that
memory slot. We can refcount slot during map/unmap and do not remove a
slot it refcount > 0, or we can kill guest if this happens. That what
real HW would do.

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Vasilis Liaskovitis]

On Wed, Mar 14, 2012 at 05:23:24PM +0200, Gleb Natapov wrote:
> On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote:
> > Hi,
> > 
> > 
> On a different note, are your going to continue working on your memory hot plug series?
> I am going to look at it now.

The memory hotplug patch from September has several issues (e.g. unplug/memory
free isn't really performed at the qemu level) and is outdated (qemu-kvm-0.15
and seabios-0.6.1 timeframe). 
I 've continued working on it against master/new memory API and I will resubmit
for comments.

thanks,

- Vasilis

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Gleb Natapov]

On Wed, Mar 14, 2012 at 08:55:08PM +0100, Vasilis Liaskovitis wrote:
> On Wed, Mar 14, 2012 at 05:23:24PM +0200, Gleb Natapov wrote:
> > On Wed, Mar 14, 2012 at 02:49:59PM +0100, Vasilis Liaskovitis wrote:
> > > Hi,
> > > 
> > > 
> > On a different note, are your going to continue working on your memory hot plug series?
> > I am going to look at it now.
> 
> The memory hotplug patch from September has several issues (e.g. unplug/memory
> free isn't really performed at the qemu level) and is outdated (qemu-kvm-0.15
> and seabios-0.6.1 timeframe). 
> I 've continued working on it against master/new memory API and I will resubmit
> for comments.
> 
Thanks, I've commented on your previous submission. When do you expect
to send update patches?

--
			Gleb.

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Anthony Liguori]

On 02/15/2012 05:16 PM, Igor Mammedov wrote:
> Convert pc cpu to qdev device that is attached to icc bus, later
> hot-plug ability of icc bus will allow to implement cpu hot-plug.
>
> Signed-off-by: Igor Mammedov<imammedo@redhat.com>

Obviously, this needs to go to QOM, but also, this is a shallow conversion. 
Beginning with a shallow conversion is okay but only if there's a clear path to 
a proper conversion.  I don't think this approach fits that requirement.  The 
approach is a bit too pc centric.  It's not clear how it would generalize.

Regards,

Anthony Liguori

> ---
>   hw/pc.c              |   62 +++++++++++++++++++++++++++++++++++++++++++------
>   target-i386/cpu.h    |    1 +
>   target-i386/helper.c |   26 ++++++++++++++------
>   3 files changed, 73 insertions(+), 16 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 33d8090..b8db5dc 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>       }
>   }
>
> +typedef struct CPUPC {
> +    ICCBusDevice busdev;
> +    char *model;
> +    CPUState state;
> +} CPUPC;
> +
>   static void pc_cpu_reset(void *opaque)
>   {
>       CPUState *env = opaque;
> @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque)
>       env->halted = !cpu_is_bsp(env);
>   }
>
> -static CPUState *pc_new_cpu(const char *cpu_model)
> +static DeviceState *pc_new_cpu(const char *cpu_model)
>   {
> -    CPUState *env;
> +    DeviceState *dev;
> +    BusState *b;
>
> -    env = cpu_init(cpu_model);
> -    if (!env) {
> -        fprintf(stderr, "Unable to find x86 CPU definition\n");
> -        exit(1);
> +    b = get_icc_bus();
> +    dev = qdev_create(b, "cpu-pc");
> +    if (!dev) {
> +        return NULL;
> +    }
> +    qdev_prop_set_string(dev, "model", g_strdup(cpu_model));
> +    if (qdev_init(dev)<  0) {
> +        return NULL;
> +    }
> +    return dev;
> +}
> +
> +static int cpu_device_init(ICCBusDevice *dev)
> +{
> +    CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
> +    CPUState *env =&cpu->state;
> +
> +    if (cpu_x86_init_inplace(env, cpu->model)<  0) {
> +        return -1;
>       }
> +
>       if ((env->cpuid_features&  CPUID_APIC) || smp_cpus>  1) {
>           env->apic_state = apic_init(env, env->cpuid_apic_id);
>       }
> -    qemu_register_reset(pc_cpu_reset, env);
> +
> +    return 0;
> +}
> +
> +static void cpu_device_reset(DeviceState *dev) {
> +    CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev);
> +    CPUState *env =&cpu->state;
>       pc_cpu_reset(env);
> -    return env;
>   }
>
> +static ICCBusDeviceInfo cpu_device_info = {
> +    .qdev.name = "cpu-pc",
> +    .qdev.size = sizeof(CPUPC),
> +    .qdev.reset = cpu_device_reset,
> +    .init = cpu_device_init,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_STRING("model", CPUPC, model),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void pc_register_devices(void)
> +{
> +    iccbus_register_devinfo(&cpu_device_info);
> +}
> +
> +device_init(pc_register_devices);
> +
>   void pc_cpus_init(const char *cpu_model)
>   {
>       int i;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 7e66bcf..830b65e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -775,6 +775,7 @@ typedef struct CPUX86State {
>   } CPUX86State;
>
>   CPUX86State *cpu_x86_init(const char *cpu_model);
> +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model);
>   int cpu_x86_exec(CPUX86State *s);
>   void cpu_x86_close(CPUX86State *s);
>   void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg);
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 2586aff..df2f5ba 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
>       return 1;
>   }
>
> -CPUX86State *cpu_x86_init(const char *cpu_model)
> +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model)
>   {
> -    CPUX86State *env;
>       static int inited;
>
> -    env = g_malloc0(sizeof(CPUX86State));
> +    if (cpu_x86_register(env, cpu_model)<  0) {
> +        fprintf(stderr, "Unable to find x86 CPU definition\n");
> +        return -1;
> +    }
>       cpu_exec_init(env);
>       env->cpu_model_str = cpu_model;
>
> @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>               cpu_set_debug_excp_handler(breakpoint_handler);
>   #endif
>       }
> -    if (cpu_x86_register(env, cpu_model)<  0) {
> -        cpu_x86_close(env);
> -        return NULL;
> -    }
>       env->cpuid_apic_id = env->cpu_index;
>       mce_init(env);
>
>       qemu_init_vcpu(env);
>
> -    return env;
> +    return 0;
>   }
>
> +CPUX86State *cpu_x86_init(const char *cpu_model)
> +{
> +    CPUX86State *env;
> +
> +    env = g_malloc0(sizeof(CPUX86State));
> +    if (cpu_x86_init_inplace(env, cpu_model)<  0) {
> +        g_free(env);
> +        return NULL;
> +    }
> +
> +    return env;
> +}
>   #if !defined(CONFIG_USER_ONLY)
>   void do_cpu_init(CPUState *env)
>   {

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Andreas Färber]

Am 16.02.2012 00:16, schrieb Igor Mammedov:
> Convert pc cpu to qdev device that is attached to icc bus, later
> hot-plug ability of icc bus will allow to implement cpu hot-plug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

This conflicts with CPU QOM'ification across targets (and no longer
applies due to type_init() introduction).

> ---
>  hw/pc.c              |   62 +++++++++++++++++++++++++++++++++++++++++++------
>  target-i386/cpu.h    |    1 +
>  target-i386/helper.c |   26 ++++++++++++++------
>  3 files changed, 73 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 33d8090..b8db5dc 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>      }
>  }
>  
> +typedef struct CPUPC {
> +    ICCBusDevice busdev;
> +    char *model;
> +    CPUState state;
> +} CPUPC;

I don't like this approach. For starters, using CPUState as field rather
than pointer seems a bad idea to me (CPUX86State would only be slightly
better). We should have a single code path through which we instantiate
CPUs, currently: cpu_$arch_init(const char *cpu_model)
With my series completed, this would return an X86CPU object.

Depending on our liking, we could either place some ICC / APIC /
whatever fields directly into that, or embed the X86CPU in an object
such as yours above as a link<X86CPU>. I do feel however that the model
string is misplaced there. Question is whether this ICC stuff is
actually part of the CPU or part of the CPU wiring on the mainboard - I
vaguely remember someone saying that this changed over time...? Having
both depending on CPU subclass might also be an option, but I'd rather
leave such decisions as a follow-up to the core QOM'ification.

Andreas

> +
>  static void pc_cpu_reset(void *opaque)
>  {
>      CPUState *env = opaque;
> @@ -930,23 +936,63 @@ static void pc_cpu_reset(void *opaque)
>      env->halted = !cpu_is_bsp(env);
>  }
>  
> -static CPUState *pc_new_cpu(const char *cpu_model)
> +static DeviceState *pc_new_cpu(const char *cpu_model)
>  {
> -    CPUState *env;
> +    DeviceState *dev;
> +    BusState *b;
>  
> -    env = cpu_init(cpu_model);
> -    if (!env) {
> -        fprintf(stderr, "Unable to find x86 CPU definition\n");
> -        exit(1);
> +    b = get_icc_bus();
> +    dev = qdev_create(b, "cpu-pc");
> +    if (!dev) {
> +        return NULL;
> +    }
> +    qdev_prop_set_string(dev, "model", g_strdup(cpu_model));
> +    if (qdev_init(dev) < 0) {
> +        return NULL;
> +    }
> +    return dev;
> +}
> +
> +static int cpu_device_init(ICCBusDevice *dev)
> +{
> +    CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
> +    CPUState *env = &cpu->state;
> +
> +    if (cpu_x86_init_inplace(env, cpu->model) < 0) {
> +        return -1;
>      }
> +
>      if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>          env->apic_state = apic_init(env, env->cpuid_apic_id);
>      }
> -    qemu_register_reset(pc_cpu_reset, env);
> +
> +    return 0;
> +}
> +
> +static void cpu_device_reset(DeviceState *dev) {
> +    CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev);
> +    CPUState *env = &cpu->state;
>      pc_cpu_reset(env);
> -    return env;
>  }
>  
> +static ICCBusDeviceInfo cpu_device_info = {
> +    .qdev.name = "cpu-pc",
> +    .qdev.size = sizeof(CPUPC),
> +    .qdev.reset = cpu_device_reset,
> +    .init = cpu_device_init,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_STRING("model", CPUPC, model),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void pc_register_devices(void)
> +{
> +    iccbus_register_devinfo(&cpu_device_info);
> +}
> +
> +device_init(pc_register_devices);
> +
>  void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 7e66bcf..830b65e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -775,6 +775,7 @@ typedef struct CPUX86State {
>  } CPUX86State;
>  
>  CPUX86State *cpu_x86_init(const char *cpu_model);
> +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model);
>  int cpu_x86_exec(CPUX86State *s);
>  void cpu_x86_close(CPUX86State *s);
>  void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg);
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 2586aff..df2f5ba 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1235,12 +1235,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
>      return 1;
>  }
>  
> -CPUX86State *cpu_x86_init(const char *cpu_model)
> +int cpu_x86_init_inplace(CPUX86State *env, const char *cpu_model)
>  {
> -    CPUX86State *env;
>      static int inited;
>  
> -    env = g_malloc0(sizeof(CPUX86State));
> +    if (cpu_x86_register(env, cpu_model) < 0) {
> +        fprintf(stderr, "Unable to find x86 CPU definition\n");
> +        return -1;
> +    }
>      cpu_exec_init(env);
>      env->cpu_model_str = cpu_model;
>  
> @@ -1253,18 +1255,26 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
>              cpu_set_debug_excp_handler(breakpoint_handler);
>  #endif
>      }
> -    if (cpu_x86_register(env, cpu_model) < 0) {
> -        cpu_x86_close(env);
> -        return NULL;
> -    }
>      env->cpuid_apic_id = env->cpu_index;
>      mce_init(env);
>  
>      qemu_init_vcpu(env);
>  
> -    return env;
> +    return 0;
>  }
>  
> +CPUX86State *cpu_x86_init(const char *cpu_model)
> +{
> +    CPUX86State *env;
> +
> +    env = g_malloc0(sizeof(CPUX86State));
> +    if (cpu_x86_init_inplace(env, cpu_model) < 0) {
> +        g_free(env);
> +        return NULL;
> +    }
> +
> +    return env;
> +}
>  #if !defined(CONFIG_USER_ONLY)
>  void do_cpu_init(CPUState *env)
>  {

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Igor Mammedov]

On 02/16/2012 05:09 PM, Andreas Färber wrote:
> Am 16.02.2012 00:16, schrieb Igor Mammedov:
>> Convert pc cpu to qdev device that is attached to icc bus, later
>> hot-plug ability of icc bus will allow to implement cpu hot-plug.
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>
> This conflicts with CPU QOM'ification across targets (and no longer
> applies due to type_init() introduction).
>
>> ---
>>   hw/pc.c              |   62 +++++++++++++++++++++++++++++++++++++++++++------
>>   target-i386/cpu.h    |    1 +
>>   target-i386/helper.c |   26 ++++++++++++++------
>>   3 files changed, 73 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 33d8090..b8db5dc 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -922,6 +922,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>>       }
>>   }
>>
>> +typedef struct CPUPC {
>> +    ICCBusDevice busdev;
>> +    char *model;
>> +    CPUState state;
>> +} CPUPC;
>
> I don't like this approach. For starters, using CPUState as field rather
> than pointer seems a bad idea to me (CPUX86State would only be slightly
> better). We should have a single code path through which we instantiate
> CPUs, currently: cpu_$arch_init(const char *cpu_model)
> With my series completed, this would return an X86CPU object.
>
> Depending on our liking, we could either place some ICC / APIC /
> whatever fields directly into that, or embed the X86CPU in an object
> such as yours above as a link<X86CPU>. I do feel however that the model
> string is misplaced there. Question is whether this ICC stuff is
> actually part of the CPU or part of the CPU wiring on the mainboard - I
> vaguely remember someone saying that this changed over time...? Having
Yep, since P4 times sysbus used instead of icc so we can just ignore icc.

> both depending on CPU subclass might also be an option, but I'd rather
> leave such decisions as a follow-up to the core QOM'ification.
>

With QOM and your work this patch is obsolete. I see you've already QOM-ified
X86CPU in your qom-cpu tree. With your permission I'll play with it and check
what could be done for cpu hot-plug feature.

-- 
Thanks,
  Igor

Re: [Qemu-devel] [PATCH 2/7] Convert pc cpu to qdev

[Andreas Färber]

Am 17.02.2012 18:16, schrieb Igor Mammedov:
> On 02/16/2012 05:09 PM, Andreas Färber wrote:
>> We should have a single code path through which we instantiate
>> CPUs, currently: cpu_$arch_init(const char *cpu_model)
>> With my series completed, this would return an X86CPU object.
>>
>> Depending on our liking, we could either place some ICC / APIC /
>> whatever fields directly into that, or embed the X86CPU in an object
>> such as yours above as a link<X86CPU>. I do feel however that the model
>> string is misplaced there. Question is whether this ICC stuff is
>> actually part of the CPU or part of the CPU wiring on the mainboard - I
>> vaguely remember someone saying that this changed over time...? Having
> Yep, since P4 times sysbus used instead of icc so we can just ignore icc.
> 
>> both depending on CPU subclass might also be an option, but I'd rather
>> leave such decisions as a follow-up to the core QOM'ification.
>>
> 
> With QOM and your work this patch is obsolete. I see you've already
> QOM-ified X86CPU in your qom-cpu tree.

Yeah, I postponed PowerPC and took a shortcut by renaming cpuid.c. ;)

> With your permission I'll play with it and
> check
> what could be done for cpu hot-plug feature.

Sure, just beware that I frequently rebase this branch based on feedback
or moving upstream.
Also note that the CPUArchState refactoring is meant only temporary (I'm
using a script for that) and should be avoided in new code in favor of
CPUX86State or X86CPU/X86CPUClass.

If you need some change to make your hotplug work easier, just let me know.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 3/7] cleanup: get rid of pc_new_cpu

[Igor Mammedov]

Since pc_new_cpu is used only once and to simplify implementation
of pc_cpus_init, pc_new_cpu body just moved into pc_cpus_init.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index b8db5dc..d9c397a 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -936,23 +936,6 @@ static void pc_cpu_reset(void *opaque)
     env->halted = !cpu_is_bsp(env);
 }
 
-static DeviceState *pc_new_cpu(const char *cpu_model)
-{
-    DeviceState *dev;
-    BusState *b;
-
-    b = get_icc_bus();
-    dev = qdev_create(b, "cpu-pc");
-    if (!dev) {
-        return NULL;
-    }
-    qdev_prop_set_string(dev, "model", g_strdup(cpu_model));
-    if (qdev_init(dev) < 0) {
-        return NULL;
-    }
-    return dev;
-}
-
 static int cpu_device_init(ICCBusDevice *dev)
 {
     CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
@@ -995,6 +978,8 @@ device_init(pc_register_devices);
 
 void pc_cpus_init(const char *cpu_model)
 {
+    DeviceState *dev;
+    BusState *b;
     int i;
 
     /* init CPUs */
@@ -1011,8 +996,17 @@ void pc_cpus_init(const char *cpu_model)
         qdev_init_nofail(d);
     }
 
+    b = get_icc_bus();
     for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model);
+        dev = qdev_create(b, "cpu-pc");
+        if (!dev) {
+            return;
+        }
+
+        qdev_prop_set_string(dev, "model", g_strdup(cpu_model));
+        if (qdev_init(dev) < 0) {
+            return;
+        }
     }
 }
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 4/7] cleanup: remove redundant pc_cpu_reset

[Igor Mammedov]

All cpus except of boot cpu should be halted after reset.
So remove redundant pc_cpu_reset and use cpu_reset instead.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c              |   11 ++---------
 target-i386/helper.c |    1 +
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index d9c397a..3d35d78 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -928,14 +928,6 @@ typedef struct CPUPC {
     CPUState state;
 } CPUPC;
 
-static void pc_cpu_reset(void *opaque)
-{
-    CPUState *env = opaque;
-
-    cpu_reset(env);
-    env->halted = !cpu_is_bsp(env);
-}
-
 static int cpu_device_init(ICCBusDevice *dev)
 {
     CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
@@ -955,7 +947,8 @@ static int cpu_device_init(ICCBusDevice *dev)
 static void cpu_device_reset(DeviceState *dev) {
     CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev);
     CPUState *env = &cpu->state;
-    pc_cpu_reset(env);
+
+    cpu_reset(env);
 }
 
 static ICCBusDeviceInfo cpu_device_info = {
diff --git a/target-i386/helper.c b/target-i386/helper.c
index df2f5ba..cd61d36 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -105,6 +105,7 @@ void cpu_reset(CPUX86State *env)
     env->dr[7] = DR7_FIXED_1;
     cpu_breakpoint_remove_all(env, BP_CPU);
     cpu_watchpoint_remove_all(env, BP_CPU);
+    env->halted = !cpu_is_bsp(env);
 }
 
 void cpu_x86_close(CPUX86State *env)
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 4/7] cleanup: remove redundant pc_cpu_reset

[Jan Kiszka]

On 2012-02-16 00:16, Igor Mammedov wrote:
> All cpus except of boot cpu should be halted after reset.
> So remove redundant pc_cpu_reset and use cpu_reset instead.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/pc.c              |   11 ++---------
>  target-i386/helper.c |    1 +
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index d9c397a..3d35d78 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -928,14 +928,6 @@ typedef struct CPUPC {
>      CPUState state;
>  } CPUPC;
>  
> -static void pc_cpu_reset(void *opaque)
> -{
> -    CPUState *env = opaque;
> -
> -    cpu_reset(env);
> -    env->halted = !cpu_is_bsp(env);
> -}
> -
>  static int cpu_device_init(ICCBusDevice *dev)
>  {
>      CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
> @@ -955,7 +947,8 @@ static int cpu_device_init(ICCBusDevice *dev)
>  static void cpu_device_reset(DeviceState *dev) {
>      CPUPC *cpu = DO_UPCAST(CPUPC, busdev.qdev, dev);
>      CPUState *env = &cpu->state;
> -    pc_cpu_reset(env);
> +
> +    cpu_reset(env);
>  }
>  
>  static ICCBusDeviceInfo cpu_device_info = {
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index df2f5ba..cd61d36 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -105,6 +105,7 @@ void cpu_reset(CPUX86State *env)
>      env->dr[7] = DR7_FIXED_1;
>      cpu_breakpoint_remove_all(env, BP_CPU);
>      cpu_watchpoint_remove_all(env, BP_CPU);
> +    env->halted = !cpu_is_bsp(env);
>  }
>  
>  void cpu_x86_close(CPUX86State *env)

See http://thread.gmane.org/gmane.comp.emulators.qemu/100806 for an
earlier attempt to clean this up. I forgot about following up on this,
but you should try to adopt the suggestions to avoid repeating that
discussion.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 5/7] Set default 'model' property if it wasn't specified yet.

[Igor Mammedov]

If cpu model wasn't specified at startup or hot-plug set it to default
value for the target.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 3d35d78..ec50f16 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -933,6 +933,14 @@ static int cpu_device_init(ICCBusDevice *dev)
     CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
     CPUState *env = &cpu->state;
 
+    if (cpu->model == NULL) {
+#ifdef TARGET_X86_64
+        qdev_prop_set_string(&dev->qdev, "model", g_strdup("qemu64"));
+#else
+        qdev_prop_set_string(&dev->qdev, "model", g_strdup("qemu32"));
+#endif
+    }
+
     if (cpu_x86_init_inplace(env, cpu->model) < 0) {
         return -1;
     }
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 5/7] Set default 'model' property if it wasn't specified yet.

[Jan Kiszka]

On 2012-02-16 00:16, Igor Mammedov wrote:
> If cpu model wasn't specified at startup or hot-plug set it to default
> value for the target.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/pc.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 3d35d78..ec50f16 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -933,6 +933,14 @@ static int cpu_device_init(ICCBusDevice *dev)
>      CPUPC* cpu = DO_UPCAST(CPUPC, busdev, dev);
>      CPUState *env = &cpu->state;
>  
> +    if (cpu->model == NULL) {
> +#ifdef TARGET_X86_64
> +        qdev_prop_set_string(&dev->qdev, "model", g_strdup("qemu64"));
> +#else
> +        qdev_prop_set_string(&dev->qdev, "model", g_strdup("qemu32"));
> +#endif
> +    }
> +
>      if (cpu_x86_init_inplace(env, cpu->model) < 0) {
>          return -1;
>      }

This obsoletes a similar logic in pc_cpus_init. Please consolidate.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 6/7] Prepare ACPI infrastructure for cpu hot-plug in acpi_piix4

[Igor Mammedov]

It's backport of acpi related cpu-hotplug code from qemu-kvm.
Provides means to communicate cpu hot-plug events to guest OS
and that works with current seabios.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi_piix4.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/pc.h         |    1 +
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index bdc55a1..5a83d73 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -43,9 +43,15 @@
 #define PCI_BASE 0xae00
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
+#define PROC_BASE 0xaf00
 
+#define PIIX4_CPU_HOTPLUG_STATUS 4
 #define PIIX4_PCI_HOTPLUG_STATUS 2
 
+struct gpe_regs {
+    uint8_t cpus_sts[32];
+};
+
 struct pci_status {
     uint32_t up;
     uint32_t down;
@@ -71,6 +77,7 @@ typedef struct PIIX4PMState {
 
     /* for pci hotplug */
     ACPIGPE gpe;
+    struct gpe_regs gpe_cpu;
     struct pci_status pci0_status;
     uint32_t pci0_hotplug_enable;
 } PIIX4PMState;
@@ -90,7 +97,8 @@ static void pm_update_sci(PIIX4PMState *s)
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                    ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
-        (((s->gpe.sts[0] & s->gpe.en[0]) & PIIX4_PCI_HOTPLUG_STATUS) != 0);
+        (((s->gpe.sts[0] & s->gpe.en[0]) &
+         (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);
 
     qemu_set_irq(s->irq, sci_level);
     /* schedule a timer interruption if needed */
@@ -329,11 +337,16 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
 
 }
 
+static PIIX4PMState *global_piix4_pm_state; /* cpu hotadd */
+
 static int piix4_pm_initfn(PCIDevice *dev)
 {
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
     uint8_t *pci_conf;
 
+    /* for cpu hotadd */
+    global_piix4_pm_state = s;
+
     pci_conf = s->dev.config;
     pci_conf[0x06] = 0x80;
     pci_conf[0x07] = 0x02;
@@ -425,8 +438,17 @@ device_init(piix4_pm_register);
 static uint32_t gpe_readb(void *opaque, uint32_t addr)
 {
     PIIX4PMState *s = opaque;
-    uint32_t val = acpi_gpe_ioport_readb(&s->gpe, addr);
+    uint32_t val = 0;
+    struct gpe_regs *g = &s->gpe_cpu;
 
+    switch (addr) {
+    case PROC_BASE ... PROC_BASE+31:
+        val = g->cpus_sts[addr - PROC_BASE];
+        break;
+    default:
+        val = acpi_gpe_ioport_readb(&s->gpe, addr);
+    }
+ 
     PIIX4_DPRINTF("gpe read %x == %x\n", addr, val);
     return val;
 }
@@ -519,11 +541,20 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
 static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 {
     struct pci_status *pci0_status = &s->pci0_status;
+    int i = 0, cpus = smp_cpus;
 
+    while (cpus > 0) {
+        s->gpe_cpu.cpus_sts[i++] = (cpus < 8) ? (1 << cpus) - 1 : 0xff;
+        cpus -= 8;
+    }
+ 
     register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
     register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
     acpi_gpe_blk(&s->gpe, GPE_BASE);
 
+    register_ioport_write(PROC_BASE, 32, 1, gpe_writeb, s);
+    register_ioport_read(PROC_BASE, 32, 1,  gpe_readb, s);
+
     register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status);
     register_ioport_read(PCI_BASE, 8, 4,  pcihotplug_read, pci0_status);
 
@@ -536,6 +567,36 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
     pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
 }
 
+static void enable_processor(PIIX4PMState *s, int cpu)
+{
+    struct gpe_regs *g = &s->gpe_cpu;
+    ACPIGPE *gpe = &s->gpe;
+
+    *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
+    g->cpus_sts[cpu / 8] |= (1 << (cpu % 8));
+}
+
+static void disable_processor(PIIX4PMState *s, int cpu)
+{
+    struct gpe_regs *g = &s->gpe_cpu;
+    ACPIGPE *gpe = &s->gpe;
+
+    *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
+    g->cpus_sts[cpu / 8] &= ~(1 << (cpu % 8));
+}
+
+void acpi_cpu_hot_plug(int cpu, int state)
+{
+    PIIX4PMState *s = global_piix4_pm_state;
+
+    if (state) {
+        enable_processor(s, cpu);
+    } else {
+        disable_processor(s, cpu);
+    }
+    pm_update_sci(s);
+}
+
 static void enable_device(PIIX4PMState *s, int slot)
 {
     s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
diff --git a/hw/pc.h b/hw/pc.h
index 13e41f1..fd1e09c 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -163,6 +163,7 @@ extern size_t acpi_tables_len;
 
 void acpi_bios_init(void);
 int acpi_table_add(const char *table_desc);
+void acpi_cpu_hot_plug(int cpu, int state);
 
 /* acpi_piix.c */
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 6/7] Prepare ACPI infrastructure for cpu hot-plug in acpi_piix4

[Jan Kiszka]

On 2012-02-16 00:16, Igor Mammedov wrote:
> It's backport of acpi related cpu-hotplug code from qemu-kvm.
> Provides means to communicate cpu hot-plug events to guest OS
> and that works with current seabios.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi_piix4.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/pc.h         |    1 +
>  2 files changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index bdc55a1..5a83d73 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -43,9 +43,15 @@
>  #define PCI_BASE 0xae00
>  #define PCI_EJ_BASE 0xae08
>  #define PCI_RMV_BASE 0xae0c
> +#define PROC_BASE 0xaf00
>  
> +#define PIIX4_CPU_HOTPLUG_STATUS 4
>  #define PIIX4_PCI_HOTPLUG_STATUS 2
>  
> +struct gpe_regs {
> +    uint8_t cpus_sts[32];
> +};
> +
>  struct pci_status {
>      uint32_t up;
>      uint32_t down;
> @@ -71,6 +77,7 @@ typedef struct PIIX4PMState {
>  
>      /* for pci hotplug */
>      ACPIGPE gpe;
> +    struct gpe_regs gpe_cpu;
>      struct pci_status pci0_status;
>      uint32_t pci0_hotplug_enable;
>  } PIIX4PMState;
> @@ -90,7 +97,8 @@ static void pm_update_sci(PIIX4PMState *s)
>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>                     ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> -        (((s->gpe.sts[0] & s->gpe.en[0]) & PIIX4_PCI_HOTPLUG_STATUS) != 0);
> +        (((s->gpe.sts[0] & s->gpe.en[0]) &
> +         (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);
>  
>      qemu_set_irq(s->irq, sci_level);
>      /* schedule a timer interruption if needed */
> @@ -329,11 +337,16 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>  
>  }
>  
> +static PIIX4PMState *global_piix4_pm_state; /* cpu hotadd */
> +

Better establish a relation between the ICC bus and the PIIX4 so that
the latter can provide this reference.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 7/7] Implement cpu hot-add using device_add monitor command

[Igor Mammedov]

Adds ability to hot-add cpus if guest was started with options
    -smp X,maxcpus=Y
where X < Y.
For simplicity sake this implementation doesn't allow to add specific
cpu but rather it adds the next not yet plugged cpu.

For adding cpu just execute following command in monitor:
    device_add cpu-pc
If guest was started with a specific cpu model, then add option model
to cmonitor command. for example:
    device_add cpu-pc,model="host"

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index ec50f16..5923549 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -44,6 +44,8 @@
 #include "ui/qemu-spice.h"
 #include "memory.h"
 #include "exec-memory.h"
+#include "cpus.h"
+#include "kvm.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -941,6 +943,10 @@ static int cpu_device_init(ICCBusDevice *dev)
 #endif
     }
 
+    if (runstate_is_running()) {
+        pause_all_vcpus();
+    }
+
     if (cpu_x86_init_inplace(env, cpu->model) < 0) {
         return -1;
     }
@@ -949,6 +955,11 @@ static int cpu_device_init(ICCBusDevice *dev)
         env->apic_state = apic_init(env, env->cpuid_apic_id);
     }
 
+    cpu_synchronize_post_init(env);
+    if (runstate_is_running()) {
+        resume_all_vcpus();
+        acpi_cpu_hot_plug(env->cpuid_apic_id, 1);
+    }
     return 0;
 }
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 7/7] Implement cpu hot-add using device_add monitor command

[Anthony Liguori]

On 02/15/2012 05:16 PM, Igor Mammedov wrote:
> Adds ability to hot-add cpus if guest was started with options
>      -smp X,maxcpus=Y
> where X<  Y.
> For simplicity sake this implementation doesn't allow to add specific
> cpu but rather it adds the next not yet plugged cpu.
>
> For adding cpu just execute following command in monitor:
>      device_add cpu-pc
> If guest was started with a specific cpu model, then add option model
> to cmonitor command. for example:
>      device_add cpu-pc,model="host"
>
> Signed-off-by: Igor Mammedov<imammedo@redhat.com>

I don't see how the code matches this description, but unless you've got 
qdev-ification patches in the earlier ones (haven't made the list yet), 
device_add is not the right place for this.

There's no such thing as "cpu-pc" either.  You should start with a cpu_add command.

Regards,

Anthony Liguori

> ---
>   hw/pc.c |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index ec50f16..5923549 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -44,6 +44,8 @@
>   #include "ui/qemu-spice.h"
>   #include "memory.h"
>   #include "exec-memory.h"
> +#include "cpus.h"
> +#include "kvm.h"
>
>   /* output Bochs bios info messages */
>   //#define DEBUG_BIOS
> @@ -941,6 +943,10 @@ static int cpu_device_init(ICCBusDevice *dev)
>   #endif
>       }
>
> +    if (runstate_is_running()) {
> +        pause_all_vcpus();
> +    }
> +
>       if (cpu_x86_init_inplace(env, cpu->model)<  0) {
>           return -1;
>       }
> @@ -949,6 +955,11 @@ static int cpu_device_init(ICCBusDevice *dev)
>           env->apic_state = apic_init(env, env->cpuid_apic_id);
>       }
>
> +    cpu_synchronize_post_init(env);
> +    if (runstate_is_running()) {
> +        resume_all_vcpus();
> +        acpi_cpu_hot_plug(env->cpuid_apic_id, 1);
> +    }
>       return 0;
>   }
>

Re: [Qemu-devel] [PATCH 7/7] Implement cpu hot-add using device_add monitor command

[Igor Mammedov]

On 02/16/2012 12:35 AM, Anthony Liguori wrote:
> On 02/15/2012 05:16 PM, Igor Mammedov wrote:
>> Adds ability to hot-add cpus if guest was started with options
>> -smp X,maxcpus=Y
>> where X< Y.
>> For simplicity sake this implementation doesn't allow to add specific
>> cpu but rather it adds the next not yet plugged cpu.
>>
>> For adding cpu just execute following command in monitor:
>> device_add cpu-pc
>> If guest was started with a specific cpu model, then add option model
>> to cmonitor command. for example:
>> device_add cpu-pc,model="host"
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>
> I don't see how the code matches this description, but unless you've got qdev-ification patches in the earlier ones (haven't made the list yet),
> device_add is not the right place for this.
Indeed, I've attempted qdev-ify cpu first (patch 2/7)

>
> There's no such thing as "cpu-pc" either. You should start with a cpu_add command.
"cpu-pc" is coming from 2/7 where cpu was qdev-ifed and I needed to call it somehow.
Looking at other attempts to qdev-ify cpus i.e. "cpu-ppc", "cpu-mips", I've just
followed trend. Is there any suggestions on how it should be called?

As for cpu_add command, v1 had patch that introduced somewhat similar cpu_set qmp
command. And Jan's opinion on it was that it's not upstream-able, so device_add was
used instead.

>
> Regards,
>
> Anthony Liguori
>
>> ---
>> hw/pc.c | 11 +++++++++++
>> 1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index ec50f16..5923549 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -44,6 +44,8 @@
>> #include "ui/qemu-spice.h"
>> #include "memory.h"
>> #include "exec-memory.h"
>> +#include "cpus.h"
>> +#include "kvm.h"
>>
>> /* output Bochs bios info messages */
>> //#define DEBUG_BIOS
>> @@ -941,6 +943,10 @@ static int cpu_device_init(ICCBusDevice *dev)
>> #endif
>> }
>>
>> + if (runstate_is_running()) {
>> + pause_all_vcpus();
>> + }
>> +
>> if (cpu_x86_init_inplace(env, cpu->model)< 0) {
>> return -1;
>> }
>> @@ -949,6 +955,11 @@ static int cpu_device_init(ICCBusDevice *dev)
>> env->apic_state = apic_init(env, env->cpuid_apic_id);
>> }
>>
>> + cpu_synchronize_post_init(env);
>> + if (runstate_is_running()) {
>> + resume_all_vcpus();
>> + acpi_cpu_hot_plug(env->cpuid_apic_id, 1);
>> + }
>> return 0;
>> }
>>
>

-- 
Thanks,
  Igor

Re: [Qemu-devel] [PATCH 7/7] Implement cpu hot-add using device_add monitor command

[Andreas Färber]

Am 16.02.2012 10:33, schrieb Igor Mammedov:
> On 02/16/2012 12:35 AM, Anthony Liguori wrote:
>> On 02/15/2012 05:16 PM, Igor Mammedov wrote:
>>> Adds ability to hot-add cpus if guest was started with options
>>> -smp X,maxcpus=Y
>>> where X< Y.
>>> For simplicity sake this implementation doesn't allow to add specific
>>> cpu but rather it adds the next not yet plugged cpu.
>>>
>>> For adding cpu just execute following command in monitor:
>>> device_add cpu-pc
>>> If guest was started with a specific cpu model, then add option model
>>> to cmonitor command. for example:
>>> device_add cpu-pc,model="host"
>>>
>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>
>> I don't see how the code matches this description, but unless you've
>> got qdev-ification patches in the earlier ones (haven't made the list
>> yet),
>> device_add is not the right place for this.
> Indeed, I've attempted qdev-ify cpu first (patch 2/7)
> 
>>
>> There's no such thing as "cpu-pc" either. You should start with a
>> cpu_add command.
> "cpu-pc" is coming from 2/7 where cpu was qdev-ifed and I needed to call
> it somehow.
> Looking at other attempts to qdev-ify cpus i.e. "cpu-ppc", "cpu-mips",
> I've just
> followed trend. Is there any suggestions on how it should be called?

There's a CPU QOM'ification series of mine on the list with which your
series conflicts conceptually. Unfortunately due to name conflicts I can
no longer incrementally post a v4 so I am converting all targets now one
after another, x86 will by the current name scheme get an X86CPU
(TYPE_X86_CPU).

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 7/7] Implement cpu hot-add using device_add monitor command

[Jan Kiszka]

On 2012-02-16 00:35, Anthony Liguori wrote:
> On 02/15/2012 05:16 PM, Igor Mammedov wrote:
>> Adds ability to hot-add cpus if guest was started with options
>>      -smp X,maxcpus=Y
>> where X<  Y.
>> For simplicity sake this implementation doesn't allow to add specific
>> cpu but rather it adds the next not yet plugged cpu.
>>
>> For adding cpu just execute following command in monitor:
>>      device_add cpu-pc
>> If guest was started with a specific cpu model, then add option model
>> to cmonitor command. for example:
>>      device_add cpu-pc,model="host"
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> 
> I don't see how the code matches this description, but unless you've got 
> qdev-ification patches in the earlier ones (haven't made the list yet), 
> device_add is not the right place for this.
> 
> There's no such thing as "cpu-pc" either.  You should start with a cpu_add command.

The proper ordering is first qdev'ify x86 CPUs, then use device_add. No
new throw-away commands, please.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux