* [Qemu-devel] [PATCH 1/3] Introduce a new bus "ICC" to connect APIC
2012-01-17 13:17 [Qemu-devel] [PATCH 0/3] Make vcpu hotplug work for qemu Igor Mammedov
@ 2012-01-17 13:17 ` Igor Mammedov
2012-01-17 13:57 ` Jan Kiszka
2012-01-17 13:17 ` [Qemu-devel] [PATCH 2/3] VCPU hotplug support Igor Mammedov
2012-01-17 13:17 ` [Qemu-devel] [PATCH 3/3] add cpu_set qmp command Igor Mammedov
2 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2012-01-17 13:17 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, pingfank, gleb
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.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Makefile.objs | 2 +-
hw/apic.c | 24 +++++++++----
hw/apic.h | 1 +
hw/icc_bus.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++
hw/icc_bus.h | 61 +++++++++++++++++++++++++++++++++
hw/pc.c | 14 +++++--
target-i386/cpu.h | 1 +
target-i386/cpuid.c | 16 +++++++++
8 files changed, 199 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..ac88f2e
--- /dev/null
+++ b/hw/icc_bus.c
@@ -0,0 +1,92 @@
+/* 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"
+
+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,
+ &cpu_sockets->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..9f10e1e
--- /dev/null
+++ b/hw/icc_bus.h
@@ -0,0 +1,61 @@
+/* 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"
+
+typedef struct CPUSockets CPUSockets;
+typedef struct ICCBus ICCBus;
+typedef struct ICCBusInfo ICCBusInfo;
+typedef struct ICCBusDevice ICCBusDevice;
+typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
+
+struct CPUSockets {
+ SysBusDevice sysdev;
+ ICCBus *icc_bus;
+};
+
+struct CPUSInfo {
+ DeviceInfo info;
+};
+
+struct ICCBus {
+ BusState qbus;
+};
+
+struct ICCBusInfo {
+ BusInfo qinfo;
+};
+struct ICCBusDevice {
+ DeviceState qdev;
+};
+
+typedef int (*iccbus_initfn)(ICCBusDevice *dev);
+
+struct ICCBusDeviceInfo {
+ DeviceInfo qdev;
+ iccbus_initfn init;
+};
+
+void iccbus_register_devinfo(ICCBusDeviceInfo *info);
+BusState *get_icc_bus(void);
+
+#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..5dd1627 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);
+int 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..3370fac 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -850,6 +850,22 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
}
}
+int cpu_has_apic_feature(const char *cpu_model)
+{
+ x86_def_t def1, *def = &def1;
+
+ memset(def, 0, sizeof(*def));
+
+ if (cpu_x86_find_by_name(def, cpu_model) < 0) {
+ return 0;
+ }
+ if (def->features & CPUID_APIC) {
+ return 1;
+ } else {
+ return 0;
+ }
+}
+
int cpu_x86_register (CPUX86State *env, const char *cpu_model)
{
x86_def_t def1, *def = &def1;
--
1.7.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] Introduce a new bus "ICC" to connect APIC
2012-01-17 13:17 ` [Qemu-devel] [PATCH 1/3] Introduce a new bus "ICC" to connect APIC Igor Mammedov
@ 2012-01-17 13:57 ` Jan Kiszka
0 siblings, 0 replies; 15+ messages in thread
From: Jan Kiszka @ 2012-01-17 13:57 UTC (permalink / raw)
To: Igor Mammedov
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com
On 2012-01-17 14:17, 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.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Makefile.objs | 2 +-
> hw/apic.c | 24 +++++++++----
> hw/apic.h | 1 +
> hw/icc_bus.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/icc_bus.h | 61 +++++++++++++++++++++++++++++++++
> hw/pc.c | 14 +++++--
> target-i386/cpu.h | 1 +
> target-i386/cpuid.c | 16 +++++++++
> 8 files changed, 199 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..ac88f2e
> --- /dev/null
> +++ b/hw/icc_bus.c
> @@ -0,0 +1,92 @@
> +/* 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"
> +
> +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,
> + &cpu_sockets->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..9f10e1e
> --- /dev/null
> +++ b/hw/icc_bus.h
> @@ -0,0 +1,61 @@
> +/* 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"
> +
> +typedef struct CPUSockets CPUSockets;
> +typedef struct ICCBus ICCBus;
> +typedef struct ICCBusInfo ICCBusInfo;
> +typedef struct ICCBusDevice ICCBusDevice;
> +typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
> +
> +struct CPUSockets {
> + SysBusDevice sysdev;
> + ICCBus *icc_bus;
> +};
> +
> +struct CPUSInfo {
> + DeviceInfo info;
> +};
> +
> +struct ICCBus {
> + BusState qbus;
> +};
> +
> +struct ICCBusInfo {
> + BusInfo qinfo;
> +};
> +struct ICCBusDevice {
> + DeviceState qdev;
> +};
> +
> +typedef int (*iccbus_initfn)(ICCBusDevice *dev);
> +
> +struct ICCBusDeviceInfo {
> + DeviceInfo qdev;
> + iccbus_initfn init;
> +};
There should be no need to make all these structures public, just keep
the typedefs as required. Some structs are even unused unless I miss
something.
> +
> +void iccbus_register_devinfo(ICCBusDeviceInfo *info);
> +BusState *get_icc_bus(void);
> +
> +#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..5dd1627 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);
> +int 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..3370fac 100644
> --- a/target-i386/cpuid.c
> +++ b/target-i386/cpuid.c
> @@ -850,6 +850,22 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
> }
> }
>
> +int cpu_has_apic_feature(const char *cpu_model)
bool cpu_has_apic_feature(...
> +{
> + x86_def_t def1, *def = &def1;
> +
> + memset(def, 0, sizeof(*def));
> +
> + if (cpu_x86_find_by_name(def, cpu_model) < 0) {
> + return 0;
> + }
> + if (def->features & CPUID_APIC) {
> + return 1;
> + } else {
> + return 0;
> + }
return def->feature & CPUID_APIC;
> +}
> +
> int cpu_x86_register (CPUX86State *env, const char *cpu_model)
> {
> x86_def_t def1, *def = &def1;
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/3] VCPU hotplug support
2012-01-17 13:17 [Qemu-devel] [PATCH 0/3] Make vcpu hotplug work for qemu Igor Mammedov
2012-01-17 13:17 ` [Qemu-devel] [PATCH 1/3] Introduce a new bus "ICC" to connect APIC Igor Mammedov
@ 2012-01-17 13:17 ` Igor Mammedov
2012-01-17 14:17 ` Jan Kiszka
2012-01-17 13:17 ` [Qemu-devel] [PATCH 3/3] add cpu_set qmp command Igor Mammedov
2 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2012-01-17 13:17 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, pingfank, gleb
Rebase of the missing bits from qemu-kvm for vcpu hotplug
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Makefile.objs | 2 +-
Makefile.target | 2 +-
hw/acpi_piix4.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
hw/pc.c | 21 +++++++++++++-
hw/pc_piix.c | 4 ++
sysemu.h | 2 +
target-i386/cpu.h | 1 +
7 files changed, 108 insertions(+), 7 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index 45df666..2a8ccc1 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 icc_bus.o
+hw-obj-$(CONFIG_ACPI) += acpi.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/Makefile.target b/Makefile.target
index 06d79b8..4865dde 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -226,7 +226,7 @@ obj-y += device-hotplug.o
# Hardware support
obj-i386-y += vga.o
obj-i386-y += mc146818rtc.o pc.o
-obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
+obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o acpi_piix4.o
obj-i386-y += vmport.o
obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
obj-i386-y += debugcon.o multiboot.o
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index bdc55a1..f1cdcc1 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -39,13 +39,19 @@
#define ACPI_DBG_IO_ADDR 0xb044
#define GPE_BASE 0xafe0
+#define PROC_BASE 0xaf00
#define GPE_LEN 4
#define PCI_BASE 0xae00
#define PCI_EJ_BASE 0xae08
#define PCI_RMV_BASE 0xae0c
+#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,9 +97,11 @@ 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 */
acpi_pm_tmr_update(&s->tmr, (s->pm1a.en & ACPI_BITMASK_TIMER_ENABLE) &&
!(pmsts & ACPI_BITMASK_TIMER_STATUS));
@@ -219,10 +228,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
{ \
.name = (stringify(_field)), \
.version_id = 0, \
- .num = GPE_LEN, \
.info = &vmstate_info_uint16, \
.size = sizeof(uint16_t), \
- .flags = VMS_ARRAY | VMS_POINTER, \
+ .flags = VMS_SINGLE | VMS_POINTER, \
.offset = vmstate_offset_pointer(_state, _field, uint8_t), \
}
@@ -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,7 +438,16 @@ 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,50 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
}
+extern const char *global_cpu_model;
+
+#ifdef TARGET_I386
+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 qemu_system_cpu_hot_add(int cpu, int state)
+{
+ CPUState *env;
+ PIIX4PMState *s = global_piix4_pm_state;
+
+ if (state && !qemu_get_cpu(cpu)) {
+ env = pc_new_cpu(global_cpu_model);
+ if (!env) {
+ fprintf(stderr, "cpu %d creation failed\n", cpu);
+ return;
+ }
+ env->cpuid_apic_id = cpu;
+ }
+
+ if (state) {
+ enable_processor(s, cpu);
+ } else {
+ disable_processor(s, cpu);
+ }
+ pm_update_sci(s);
+}
+#endif
+
static void enable_device(PIIX4PMState *s, int slot)
{
s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
diff --git a/hw/pc.c b/hw/pc.c
index 33d8090..c7342d8 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
@@ -930,10 +932,22 @@ static void pc_cpu_reset(void *opaque)
env->halted = !cpu_is_bsp(env);
}
-static CPUState *pc_new_cpu(const char *cpu_model)
+CPUState *pc_new_cpu(const char *cpu_model)
{
CPUState *env;
+ if (cpu_model == NULL) {
+#ifdef TARGET_X86_64
+ cpu_model = "qemu64";
+#else
+ cpu_model = "qemu32";
+#endif
+ }
+
+ if (runstate_is_running()) {
+ pause_all_vcpus();
+ }
+
env = cpu_init(cpu_model);
if (!env) {
fprintf(stderr, "Unable to find x86 CPU definition\n");
@@ -944,6 +958,11 @@ static CPUState *pc_new_cpu(const char *cpu_model)
}
qemu_register_reset(pc_cpu_reset, env);
pc_cpu_reset(env);
+
+ cpu_synchronize_post_init(env);
+ if (runstate_is_running()) {
+ resume_all_vcpus();
+ }
return env;
}
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index b70431f..1c77ea1 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -53,6 +53,8 @@ static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
+const char *global_cpu_model; /* cpu hotadd */
+
static void ioapic_init(GSIState *gsi_state)
{
DeviceState *dev;
@@ -102,6 +104,8 @@ static void pc_init1(MemoryRegion *system_memory,
MemoryRegion *rom_memory;
DeviceState *dev;
+ global_cpu_model = cpu_model;
+
pc_cpus_init(cpu_model);
if (kvmclock_enabled) {
diff --git a/sysemu.h b/sysemu.h
index ddef2bb..678b478 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -155,6 +155,8 @@ void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
int do_pcie_aer_inject_error(Monitor *mon,
const QDict *qdict, QObject **ret_data);
+void qemu_system_cpu_hot_add(int cpu, int state);
+
/* serial ports */
#define MAX_SERIAL_PORTS 4
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 5dd1627..a9a6136 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -931,6 +931,7 @@ void cpu_x86_update_cr4(CPUX86State *env, uint32_t new_cr4);
/* hw/pc.c */
void cpu_smm_update(CPUX86State *env);
uint64_t cpu_get_tsc(CPUX86State *env);
+CPUState *pc_new_cpu(const char *cpu_model);
/* used to debug */
#define X86_DUMP_FPU 0x0001 /* dump FPU state too */
--
1.7.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 2/3] VCPU hotplug support
2012-01-17 13:17 ` [Qemu-devel] [PATCH 2/3] VCPU hotplug support Igor Mammedov
@ 2012-01-17 14:17 ` Jan Kiszka
2012-01-17 14:22 ` Jan Kiszka
2012-01-23 16:29 ` Igor Mammedov
0 siblings, 2 replies; 15+ messages in thread
From: Jan Kiszka @ 2012-01-17 14:17 UTC (permalink / raw)
To: Igor Mammedov
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com
On 2012-01-17 14:17, Igor Mammedov wrote:
> Rebase of the missing bits from qemu-kvm for vcpu hotplug
Description, please. Please try to split up, at least into PIIX4
preparations and "the rest". Maybe also a patch for a generic CPU
hotplug infrastructure.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Makefile.objs | 2 +-
> Makefile.target | 2 +-
> hw/acpi_piix4.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> hw/pc.c | 21 +++++++++++++-
> hw/pc_piix.c | 4 ++
> sysemu.h | 2 +
> target-i386/cpu.h | 1 +
> 7 files changed, 108 insertions(+), 7 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 45df666..2a8ccc1 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 icc_bus.o
> +hw-obj-$(CONFIG_ACPI) += acpi.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/Makefile.target b/Makefile.target
> index 06d79b8..4865dde 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -226,7 +226,7 @@ obj-y += device-hotplug.o
> # Hardware support
> obj-i386-y += vga.o
> obj-i386-y += mc146818rtc.o pc.o
> -obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
> +obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o acpi_piix4.o
That's a qemu-kvm hack, see below for the discussion. No-go for upstream
- likely.
> obj-i386-y += vmport.o
> obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
> obj-i386-y += debugcon.o multiboot.o
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index bdc55a1..f1cdcc1 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -39,13 +39,19 @@
> #define ACPI_DBG_IO_ADDR 0xb044
>
> #define GPE_BASE 0xafe0
> +#define PROC_BASE 0xaf00
> #define GPE_LEN 4
> #define PCI_BASE 0xae00
> #define PCI_EJ_BASE 0xae08
> #define PCI_RMV_BASE 0xae0c
>
> +#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,9 +97,11 @@ 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 */
> acpi_pm_tmr_update(&s->tmr, (s->pm1a.en & ACPI_BITMASK_TIMER_ENABLE) &&
> !(pmsts & ACPI_BITMASK_TIMER_STATUS));
> @@ -219,10 +228,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
> { \
> .name = (stringify(_field)), \
> .version_id = 0, \
> - .num = GPE_LEN, \
> .info = &vmstate_info_uint16, \
> .size = sizeof(uint16_t), \
> - .flags = VMS_ARRAY | VMS_POINTER, \
> + .flags = VMS_SINGLE | VMS_POINTER, \
Does this make the vmstate incompatible to the current version?
> .offset = vmstate_offset_pointer(_state, _field, uint8_t), \
> }
>
> @@ -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,7 +438,16 @@ 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,50 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
> pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
> }
>
> +extern const char *global_cpu_model;
> +
> +#ifdef TARGET_I386
Why only TARGET_I386? If the PIIX4 is used on other targets, the
infrastructure should be prepared to enable hotplugging for them as well
(at a later point). pc_new_cpu is a bad name then. And APIC fiddling
should be pushed into the arch-specific new-cpu handler.
Also note that target dependencies are a no-go for hwlib compilations
which is clearly preferrable today.
> +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));
"cpu / 8", "cpu % 8", please. Here and below. checkpatch doesn't complain?
> +}
> +
> +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 qemu_system_cpu_hot_add(int cpu, int state)
> +{
> + CPUState *env;
> + PIIX4PMState *s = global_piix4_pm_state;
> +
> + if (state && !qemu_get_cpu(cpu)) {
> + env = pc_new_cpu(global_cpu_model);
> + if (!env) {
> + fprintf(stderr, "cpu %d creation failed\n", cpu);
> + return;
> + }
> + env->cpuid_apic_id = cpu;
See comment above about proper target abstraction.
> + }
> +
> + if (state) {
> + enable_processor(s, cpu);
> + } else {
> + disable_processor(s, cpu);
> + }
> + pm_update_sci(s);
> +}
> +#endif
> +
> static void enable_device(PIIX4PMState *s, int slot)
> {
> s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> diff --git a/hw/pc.c b/hw/pc.c
> index 33d8090..c7342d8 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"
kvm? Likely not what you want.
>
> /* output Bochs bios info messages */
> //#define DEBUG_BIOS
> @@ -930,10 +932,22 @@ static void pc_cpu_reset(void *opaque)
> env->halted = !cpu_is_bsp(env);
> }
>
> -static CPUState *pc_new_cpu(const char *cpu_model)
> +CPUState *pc_new_cpu(const char *cpu_model)
> {
> CPUState *env;
>
> + if (cpu_model == NULL) {
> +#ifdef TARGET_X86_64
> + cpu_model = "qemu64";
> +#else
> + cpu_model = "qemu32";
> +#endif
Just always initialize global_cpu_model to a non-NULL value.
> + }
> +
> + if (runstate_is_running()) {
> + pause_all_vcpus();
> + }
> +
> env = cpu_init(cpu_model);
> if (!env) {
> fprintf(stderr, "Unable to find x86 CPU definition\n");
> @@ -944,6 +958,11 @@ static CPUState *pc_new_cpu(const char *cpu_model)
> }
> qemu_register_reset(pc_cpu_reset, env);
> pc_cpu_reset(env);
> +
> + cpu_synchronize_post_init(env);
> + if (runstate_is_running()) {
> + resume_all_vcpus();
> + }
> return env;
> }
>
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index b70431f..1c77ea1 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -53,6 +53,8 @@ static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
> static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
> static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
>
> +const char *global_cpu_model; /* cpu hotadd */
> +
> static void ioapic_init(GSIState *gsi_state)
> {
> DeviceState *dev;
> @@ -102,6 +104,8 @@ static void pc_init1(MemoryRegion *system_memory,
> MemoryRegion *rom_memory;
> DeviceState *dev;
>
> + global_cpu_model = cpu_model;
> +
Move this into pc_cpus_init, and you automatically resolve the cpu_model
== NULL issue.
> pc_cpus_init(cpu_model);
>
> if (kvmclock_enabled) {
> diff --git a/sysemu.h b/sysemu.h
> index ddef2bb..678b478 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -155,6 +155,8 @@ void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
> int do_pcie_aer_inject_error(Monitor *mon,
> const QDict *qdict, QObject **ret_data);
>
> +void qemu_system_cpu_hot_add(int cpu, int state);
> +
> /* serial ports */
>
> #define MAX_SERIAL_PORTS 4
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 5dd1627..a9a6136 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -931,6 +931,7 @@ void cpu_x86_update_cr4(CPUX86State *env, uint32_t new_cr4);
> /* hw/pc.c */
> void cpu_smm_update(CPUX86State *env);
> uint64_t cpu_get_tsc(CPUX86State *env);
> +CPUState *pc_new_cpu(const char *cpu_model);
"cpu_new" or so would be better. Every target supporting hotplug would
have to implement it.
>
> /* used to debug */
> #define X86_DUMP_FPU 0x0001 /* dump FPU state too */
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 2/3] VCPU hotplug support
2012-01-17 14:17 ` Jan Kiszka
@ 2012-01-17 14:22 ` Jan Kiszka
2012-01-23 16:29 ` Igor Mammedov
1 sibling, 0 replies; 15+ messages in thread
From: Jan Kiszka @ 2012-01-17 14:22 UTC (permalink / raw)
To: Igor Mammedov
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com
On 2012-01-17 15:17, Jan Kiszka wrote:
>>
>> /* output Bochs bios info messages */
>> //#define DEBUG_BIOS
>> @@ -930,10 +932,22 @@ static void pc_cpu_reset(void *opaque)
>> env->halted = !cpu_is_bsp(env);
>> }
>>
>> -static CPUState *pc_new_cpu(const char *cpu_model)
>> +CPUState *pc_new_cpu(const char *cpu_model)
>> {
>> CPUState *env;
>>
>> + if (cpu_model == NULL) {
>> +#ifdef TARGET_X86_64
>> + cpu_model = "qemu64";
>> +#else
>> + cpu_model = "qemu32";
>> +#endif
>
> Just always initialize global_cpu_model to a non-NULL value.
Another interface idea: cache the first cpu_model in this function and
use the cached one if a later call passes in NULL. That way PIIX4 would
not need to have access to an ugly global variable.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 2/3] VCPU hotplug support
2012-01-17 14:17 ` Jan Kiszka
2012-01-17 14:22 ` Jan Kiszka
@ 2012-01-23 16:29 ` Igor Mammedov
2012-01-23 17:55 ` Jan Kiszka
1 sibling, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2012-01-23 16:29 UTC (permalink / raw)
To: Jan Kiszka
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com
On 01/17/2012 03:17 PM, Jan Kiszka wrote:
> On 2012-01-17 14:17, Igor Mammedov wrote:
>> Rebase of the missing bits from qemu-kvm for vcpu hotplug
>
> Description, please. Please try to split up, at least into PIIX4
> preparations and "the rest". Maybe also a patch for a generic CPU
> hotplug infrastructure.
>
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> ---
>> Makefile.objs | 2 +-
>> Makefile.target | 2 +-
>> hw/acpi_piix4.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>> hw/pc.c | 21 +++++++++++++-
>> hw/pc_piix.c | 4 ++
>> sysemu.h | 2 +
>> target-i386/cpu.h | 1 +
>> 7 files changed, 108 insertions(+), 7 deletions(-)
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 45df666..2a8ccc1 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 icc_bus.o
>> +hw-obj-$(CONFIG_ACPI) += acpi.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/Makefile.target b/Makefile.target
>> index 06d79b8..4865dde 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -226,7 +226,7 @@ obj-y += device-hotplug.o
>> # Hardware support
>> obj-i386-y += vga.o
>> obj-i386-y += mc146818rtc.o pc.o
>> -obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
>> +obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o acpi_piix4.o
>
> That's a qemu-kvm hack, see below for the discussion. No-go for upstream
> - likely.
>
>> obj-i386-y += vmport.o
>> obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
>> obj-i386-y += debugcon.o multiboot.o
>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>> index bdc55a1..f1cdcc1 100644
>> --- a/hw/acpi_piix4.c
>> +++ b/hw/acpi_piix4.c
>> @@ -39,13 +39,19 @@
>> #define ACPI_DBG_IO_ADDR 0xb044
>>
>> #define GPE_BASE 0xafe0
>> +#define PROC_BASE 0xaf00
>> #define GPE_LEN 4
>> #define PCI_BASE 0xae00
>> #define PCI_EJ_BASE 0xae08
>> #define PCI_RMV_BASE 0xae0c
>>
>> +#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,9 +97,11 @@ 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 */
>> acpi_pm_tmr_update(&s->tmr, (s->pm1a.en& ACPI_BITMASK_TIMER_ENABLE)&&
>> !(pmsts& ACPI_BITMASK_TIMER_STATUS));
>> @@ -219,10 +228,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
>> { \
>> .name = (stringify(_field)), \
>> .version_id = 0, \
>> - .num = GPE_LEN, \
>> .info =&vmstate_info_uint16, \
>> .size = sizeof(uint16_t), \
>> - .flags = VMS_ARRAY | VMS_POINTER, \
>> + .flags = VMS_SINGLE | VMS_POINTER, \
>
> Does this make the vmstate incompatible to the current version?
I suspect it makes it incompatible, but not sure what to do about it.
According to b2e4a396f7 in qemu-kvm it fixes migration bug. It probably
should be an independed patch not related to hotplug since qemu.git has
commit 23910d3f6 that introduced the code there and it applies to gpe
struct in general.
>
>> .offset = vmstate_offset_pointer(_state, _field, uint8_t), \
>> }
>>
>> @@ -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,7 +438,16 @@ 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,50 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
>> pci_bus_hotplug(bus, piix4_device_hotplug,&s->dev.qdev);
>> }
>>
>> +extern const char *global_cpu_model;
>> +
>> +#ifdef TARGET_I386
>
> Why only TARGET_I386? If the PIIX4 is used on other targets, the
May be there is no sence in spending time on abstacting PIIX4 that
will be used only by x86 target. Is there an even remote chance that PIIX4
could/will be used with other targets?
> infrastructure should be prepared to enable hotplugging for them as well
> (at a later point). pc_new_cpu is a bad name then. And APIC fiddling
> should be pushed into the arch-specific new-cpu handler.
I've started to implement 'new-cpu' as you suggested but could you clarify what
you've meant under "APIC fiddling"?
Is there an arch specific place for pc like hw to put this in (pc_piix.c perhaps)?
> Also note that target dependencies are a no-go for hwlib compilations
> which is clearly preferrable today.
I guess that target specific deps are here because of the code below
needed access to CPUState of specific target, with proper abstraction
this deps could be avoided.
>
>> +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));
>
> "cpu / 8", "cpu % 8", please. Here and below. checkpatch doesn't complain?
checkpatch was happy, I'll fix it.
>
>> +}
>> +
>> +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 qemu_system_cpu_hot_add(int cpu, int state)
>> +{
>> + CPUState *env;
>> + PIIX4PMState *s = global_piix4_pm_state;
>> +
>> + if (state&& !qemu_get_cpu(cpu)) {
>> + env = pc_new_cpu(global_cpu_model);
>> + if (!env) {
>> + fprintf(stderr, "cpu %d creation failed\n", cpu);
>> + return;
>> + }
>> + env->cpuid_apic_id = cpu;
>
> See comment above about proper target abstraction.
>
Let suppose that we have on command line option "-smp 1,maxcpus=3"
and then call
qemu_system_cpu_hot_add( 2, 1)
qemu_system_cpu_hot_add( 2, 1)
and we end up with
[cpu_index = 1, cpuid_apic_id = 2],
[cpu_index = 2, cpuid_apic_id = 2]
qemu_get_cpu(cpu) uses cpu_index field to lookup vcpu and
cpu_init -> cpu_x86_init -> cpu_exec_init doesn't have an ability
to create vcpu with specific cpu_index and numbers them according
to position in vcpu list. So we end up with 2 new vcpus with the
same cpuid_apic_id. That for sure might confuse guest since cpuid
for last 2 cpus will return the same value. And I'm not sure what
will happen if we call
qemu_system_cpu_hot_add( 2, 1)
qemu_system_cpu_hot_add( 1, 1)
and end up with
[cpu_index = 1, cpuid_apic_id = 2],
[cpu_index = 2, cpuid_apic_id = 1]
I don't know what kind of trouble this could cause.
It seams that "env->cpuid_apic_id = cpu" is pointless especcialy
taking in account that in cpu_x86_init cpuid_apic_id is initialized
by cpu_index.
What we gain in having cpuid_apic_id that actually duplicate cpu_index?
May be there is sence to get rid of cpuid_apic_id?
Another question is about how hot-plug/unplug should be designed:
1st approach:
With the current code we can't create vcpu with specific index.
But we can implement xen like approach, where hot-plug command says
which amount of active vcpus guest should have. This way we can
leave current cpu_init -> cpu_x86_init -> cpu_exec_init call
chain without change and plug/unplug next/last vcpu.
2nd approach:
Ability to plug/unplug individual vcpus based on their cpu_index.
to do this we need add cpu_index argument to cpu_init ->
cpu_x86_init -> cpu_exec_init call chain. It will look more
like the real hardware cpu hot-plug, but do virtual guests really
need it. And what for if this way is more preferrable than the 1st.
>> + }
>> +
>> + if (state) {
>> + enable_processor(s, cpu);
>> + } else {
>> + disable_processor(s, cpu);
>> + }
>> + pm_update_sci(s);
>> +}
>> +#endif
>> +
>> static void enable_device(PIIX4PMState *s, int slot)
>> {
>> s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 33d8090..c7342d8 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"
>
> kvm? Likely not what you want.
cpu_synchronize_post_init is declared in kvm.h
Any suggestions are welcome.
>
>>
>> /* output Bochs bios info messages */
>> //#define DEBUG_BIOS
>> @@ -930,10 +932,22 @@ static void pc_cpu_reset(void *opaque)
>> env->halted = !cpu_is_bsp(env);
>> }
>>
>> -static CPUState *pc_new_cpu(const char *cpu_model)
>> +CPUState *pc_new_cpu(const char *cpu_model)
>> {
>> CPUState *env;
>>
>> + if (cpu_model == NULL) {
>> +#ifdef TARGET_X86_64
>> + cpu_model = "qemu64";
>> +#else
>> + cpu_model = "qemu32";
>> +#endif
>
> Just always initialize global_cpu_model to a non-NULL value.
>
>> + }
>> +
>> + if (runstate_is_running()) {
>> + pause_all_vcpus();
>> + }
>> +
>> env = cpu_init(cpu_model);
>> if (!env) {
>> fprintf(stderr, "Unable to find x86 CPU definition\n");
>> @@ -944,6 +958,11 @@ static CPUState *pc_new_cpu(const char *cpu_model)
>> }
>> qemu_register_reset(pc_cpu_reset, env);
>> pc_cpu_reset(env);
>> +
>> + cpu_synchronize_post_init(env);
>> + if (runstate_is_running()) {
>> + resume_all_vcpus();
>> + }
>> return env;
>> }
--
Thanks,
Igor
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 2/3] VCPU hotplug support
2012-01-23 16:29 ` Igor Mammedov
@ 2012-01-23 17:55 ` Jan Kiszka
2012-01-24 16:24 ` Igor Mammedov
0 siblings, 1 reply; 15+ messages in thread
From: Jan Kiszka @ 2012-01-23 17:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com, Juan Quintela
On 2012-01-23 17:29, Igor Mammedov wrote:
> On 01/17/2012 03:17 PM, Jan Kiszka wrote:
>> On 2012-01-17 14:17, Igor Mammedov wrote:
>>> Rebase of the missing bits from qemu-kvm for vcpu hotplug
>>
>> Description, please. Please try to split up, at least into PIIX4
>> preparations and "the rest". Maybe also a patch for a generic CPU
>> hotplug infrastructure.
>>
>>>
>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>> ---
>>> Makefile.objs | 2 +-
>>> Makefile.target | 2 +-
>>> hw/acpi_piix4.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>> hw/pc.c | 21 +++++++++++++-
>>> hw/pc_piix.c | 4 ++
>>> sysemu.h | 2 +
>>> target-i386/cpu.h | 1 +
>>> 7 files changed, 108 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index 45df666..2a8ccc1 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 icc_bus.o
>>> +hw-obj-$(CONFIG_ACPI) += acpi.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/Makefile.target b/Makefile.target
>>> index 06d79b8..4865dde 100644
>>> --- a/Makefile.target
>>> +++ b/Makefile.target
>>> @@ -226,7 +226,7 @@ obj-y += device-hotplug.o
>>> # Hardware support
>>> obj-i386-y += vga.o
>>> obj-i386-y += mc146818rtc.o pc.o
>>> -obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o
>>> +obj-i386-y += cirrus_vga.o sga.o apic.o ioapic.o piix_pci.o acpi_piix4.o
>>
>> That's a qemu-kvm hack, see below for the discussion. No-go for upstream
>> - likely.
>>
>>> obj-i386-y += vmport.o
>>> obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
>>> obj-i386-y += debugcon.o multiboot.o
>>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>>> index bdc55a1..f1cdcc1 100644
>>> --- a/hw/acpi_piix4.c
>>> +++ b/hw/acpi_piix4.c
>>> @@ -39,13 +39,19 @@
>>> #define ACPI_DBG_IO_ADDR 0xb044
>>>
>>> #define GPE_BASE 0xafe0
>>> +#define PROC_BASE 0xaf00
>>> #define GPE_LEN 4
>>> #define PCI_BASE 0xae00
>>> #define PCI_EJ_BASE 0xae08
>>> #define PCI_RMV_BASE 0xae0c
>>>
>>> +#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,9 +97,11 @@ 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 */
>>> acpi_pm_tmr_update(&s->tmr, (s->pm1a.en& ACPI_BITMASK_TIMER_ENABLE)&&
>>> !(pmsts& ACPI_BITMASK_TIMER_STATUS));
>>> @@ -219,10 +228,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
>>> { \
>>> .name = (stringify(_field)), \
>>> .version_id = 0, \
>>> - .num = GPE_LEN, \
>>> .info =&vmstate_info_uint16, \
>>> .size = sizeof(uint16_t), \
>>> - .flags = VMS_ARRAY | VMS_POINTER, \
>>> + .flags = VMS_SINGLE | VMS_POINTER, \
>>
>> Does this make the vmstate incompatible to the current version?
> I suspect it makes it incompatible, but not sure what to do about it.
> According to b2e4a396f7 in qemu-kvm it fixes migration bug. It probably
> should be an independed patch not related to hotplug since qemu.git has
> commit 23910d3f6 that introduced the code there and it applies to gpe
> struct in general.
So this hunks converts an array back to a single uint16_t entry - unless
I'm now totally confused about VMSTATE_GPE_ARRAY. CC'ing Juan in the
hope he can parse it for us. That might be a bug in upstream then. But
it should definitely be address in a separate patch.
>
>>
>>> .offset = vmstate_offset_pointer(_state, _field, uint8_t), \
>>> }
>>>
>>> @@ -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,7 +438,16 @@ 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,50 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
>>> pci_bus_hotplug(bus, piix4_device_hotplug,&s->dev.qdev);
>>> }
>>>
>>> +extern const char *global_cpu_model;
>>> +
>>> +#ifdef TARGET_I386
>>
>> Why only TARGET_I386? If the PIIX4 is used on other targets, the
>
> May be there is no sence in spending time on abstacting PIIX4 that
> will be used only by x86 target. Is there an even remote chance that PIIX4
> could/will be used with other targets?
It's used on MIPS. At least it is built for that target.
>
>
>> infrastructure should be prepared to enable hotplugging for them as well
>> (at a later point). pc_new_cpu is a bad name then. And APIC fiddling
>> should be pushed into the arch-specific new-cpu handler.
>
> I've started to implement 'new-cpu' as you suggested but could you clarify what
> you've meant under "APIC fiddling"?
I meant cpuid_apic_id initialization.
> Is there an arch specific place for pc like hw to put this in (pc_piix.c perhaps)?
Generic PC hardware bits should go to pc.c.
>
>> Also note that target dependencies are a no-go for hwlib compilations
>> which is clearly preferrable today.
> I guess that target specific deps are here because of the code below
> needed access to CPUState of specific target, with proper abstraction
> this deps could be avoided.
Yep, that would be good.
>
>>
>>> +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));
>>
>> "cpu / 8", "cpu % 8", please. Here and below. checkpatch doesn't complain?
>
> checkpatch was happy, I'll fix it.
>
>>
>>> +}
>>> +
>>> +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 qemu_system_cpu_hot_add(int cpu, int state)
>>> +{
>>> + CPUState *env;
>>> + PIIX4PMState *s = global_piix4_pm_state;
>>> +
>>> + if (state&& !qemu_get_cpu(cpu)) {
>>> + env = pc_new_cpu(global_cpu_model);
>>> + if (!env) {
>>> + fprintf(stderr, "cpu %d creation failed\n", cpu);
>>> + return;
>>> + }
>>> + env->cpuid_apic_id = cpu;
>>
>> See comment above about proper target abstraction.
>>
>
> Let suppose that we have on command line option "-smp 1,maxcpus=3"
> and then call
> qemu_system_cpu_hot_add( 2, 1)
> qemu_system_cpu_hot_add( 2, 1)
> and we end up with
> [cpu_index = 1, cpuid_apic_id = 2],
> [cpu_index = 2, cpuid_apic_id = 2]
> qemu_get_cpu(cpu) uses cpu_index field to lookup vcpu and
> cpu_init -> cpu_x86_init -> cpu_exec_init doesn't have an ability
> to create vcpu with specific cpu_index and numbers them according
> to position in vcpu list. So we end up with 2 new vcpus with the
> same cpuid_apic_id. That for sure might confuse guest since cpuid
> for last 2 cpus will return the same value. And I'm not sure what
> will happen if we call
> qemu_system_cpu_hot_add( 2, 1)
> qemu_system_cpu_hot_add( 1, 1)
> and end up with
> [cpu_index = 1, cpuid_apic_id = 2],
> [cpu_index = 2, cpuid_apic_id = 1]
> I don't know what kind of trouble this could cause.
>
> It seams that "env->cpuid_apic_id = cpu" is pointless especcialy
> taking in account that in cpu_x86_init cpuid_apic_id is initialized
> by cpu_index.
> What we gain in having cpuid_apic_id that actually duplicate cpu_index?
> May be there is sence to get rid of cpuid_apic_id?
cpu_index is for internal counting (I think to remember that,
cpuid_apic_id is the ID exposed to the guest. During CPU hotplug, you
can control this by virtually plugging the CPU in a specific slot. So we
need to pass this ID down the init chain - just not set it in generic code.
>
> Another question is about how hot-plug/unplug should be designed:
> 1st approach:
> With the current code we can't create vcpu with specific index.
Forget about index, the apic_id is important to control. And that could
become something like -cpu XXX,apid_id=N. Of course, collisions need to
be detected and rejected.
> But we can implement xen like approach, where hot-plug command says
> which amount of active vcpus guest should have. This way we can
> leave current cpu_init -> cpu_x86_init -> cpu_exec_init call
> chain without change and plug/unplug next/last vcpu.
>
> 2nd approach:
> Ability to plug/unplug individual vcpus based on their cpu_index.
> to do this we need add cpu_index argument to cpu_init ->
> cpu_x86_init -> cpu_exec_init call chain. It will look more
> like the real hardware cpu hot-plug, but do virtual guests really
> need it. And what for if this way is more preferrable than the 1st.
>
>>> + }
>>> +
>>> + if (state) {
>>> + enable_processor(s, cpu);
>>> + } else {
>>> + disable_processor(s, cpu);
>>> + }
>>> + pm_update_sci(s);
>>> +}
>>> +#endif
>>> +
>>> static void enable_device(PIIX4PMState *s, int slot)
>>> {
>>> s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index 33d8090..c7342d8 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"
>>
>> kvm? Likely not what you want.
> cpu_synchronize_post_init is declared in kvm.h
> Any suggestions are welcome.
Indeed. Ugly. Guess we should move prototypes to cpus.h, probably
uninlining the generic helpers. But that's a separate story. For now
your version is OK then.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 2/3] VCPU hotplug support
2012-01-23 17:55 ` Jan Kiszka
@ 2012-01-24 16:24 ` Igor Mammedov
2012-01-24 16:37 ` Jan Kiszka
0 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2012-01-24 16:24 UTC (permalink / raw)
To: Jan Kiszka
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com, Juan Quintela
On 01/23/2012 06:55 PM, Jan Kiszka wrote:
> On 2012-01-23 17:29, Igor Mammedov wrote:
>> On 01/17/2012 03:17 PM, Jan Kiszka wrote:
>>
>> It seams that "env->cpuid_apic_id = cpu" is pointless especcialy
>> taking in account that in cpu_x86_init cpuid_apic_id is initialized
>> by cpu_index.
>> What we gain in having cpuid_apic_id that actually duplicate cpu_index?
>> May be there is sence to get rid of cpuid_apic_id?
>
> cpu_index is for internal counting (I think to remember that,
> cpuid_apic_id is the ID exposed to the guest. During CPU hotplug, you
> can control this by virtually plugging the CPU in a specific slot. So we
> need to pass this ID down the init chain - just not set it in generic code.
It could be set in target specific new_cpu, it is not necessary to change
whole cpu_init call chain and affect all other targets that might be not
interested in this change at all.
>
>>
>> Another question is about how hot-plug/unplug should be designed:
>> 1st approach:
>> With the current code we can't create vcpu with specific index.
>
> Forget about index, the apic_id is important to control. And that could
> become something like -cpu XXX,apid_id=N. Of course, collisions need to
> be detected and rejected.
>
>> But we can implement xen like approach, where hot-plug command says
>> which amount of active vcpus guest should have. This way we can
>> leave current cpu_init -> cpu_x86_init -> cpu_exec_init call
>> chain without change and plug/unplug next/last vcpu.
>>
>> 2nd approach:
>> Ability to plug/unplug individual vcpus based on their cpu_index.
>> to do this we need add cpu_index argument to cpu_init ->
>> cpu_x86_init -> cpu_exec_init call chain. It will look more
>> like the real hardware cpu hot-plug, but do virtual guests really
>> need it. And what for if this way is more preferrable than the 1st.
>>
Jan,
Am I right in assuming that you are in a favor of 2nd approach with correction
that some opaque cpu ID (in case of x86 it will be apic_id) will be passed down
to new_cpu?
--
Thanks,
Igor
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] VCPU hotplug support
2012-01-24 16:24 ` Igor Mammedov
@ 2012-01-24 16:37 ` Jan Kiszka
0 siblings, 0 replies; 15+ messages in thread
From: Jan Kiszka @ 2012-01-24 16:37 UTC (permalink / raw)
To: Igor Mammedov
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com, Juan Quintela
On 2012-01-24 17:24, Igor Mammedov wrote:
> On 01/23/2012 06:55 PM, Jan Kiszka wrote:
>> On 2012-01-23 17:29, Igor Mammedov wrote:
>>> On 01/17/2012 03:17 PM, Jan Kiszka wrote:
>>>
>>> It seams that "env->cpuid_apic_id = cpu" is pointless especcialy
>>> taking in account that in cpu_x86_init cpuid_apic_id is initialized
>>> by cpu_index.
>>> What we gain in having cpuid_apic_id that actually duplicate cpu_index?
>>> May be there is sence to get rid of cpuid_apic_id?
>>
>> cpu_index is for internal counting (I think to remember that,
>> cpuid_apic_id is the ID exposed to the guest. During CPU hotplug, you
>> can control this by virtually plugging the CPU in a specific slot. So we
>> need to pass this ID down the init chain - just not set it in generic code.
>
> It could be set in target specific new_cpu, it is not necessary to change
> whole cpu_init call chain and affect all other targets that might be not
> interested in this change at all.
>
>>
>>>
>>> Another question is about how hot-plug/unplug should be designed:
>>> 1st approach:
>>> With the current code we can't create vcpu with specific index.
>>
>> Forget about index, the apic_id is important to control. And that could
>> become something like -cpu XXX,apid_id=N. Of course, collisions need to
>> be detected and rejected.
>>
>>> But we can implement xen like approach, where hot-plug command says
>>> which amount of active vcpus guest should have. This way we can
>>> leave current cpu_init -> cpu_x86_init -> cpu_exec_init call
>>> chain without change and plug/unplug next/last vcpu.
>>>
>>> 2nd approach:
>>> Ability to plug/unplug individual vcpus based on their cpu_index.
>>> to do this we need add cpu_index argument to cpu_init ->
>>> cpu_x86_init -> cpu_exec_init call chain. It will look more
>>> like the real hardware cpu hot-plug, but do virtual guests really
>>> need it. And what for if this way is more preferrable than the 1st.
>>>
>
> Jan,
>
> Am I right in assuming that you are in a favor of 2nd approach with correction
> that some opaque cpu ID (in case of x86 it will be apic_id) will be passed down
> to new_cpu?
I'm in favor of "device_add <cputype>,apic_id=<N>" and that the apic_id
becomes a qdev property. That way you would not need to pass anything
down, the device (cpu) init code could pick it up on its own.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 3/3] add cpu_set qmp command
2012-01-17 13:17 [Qemu-devel] [PATCH 0/3] Make vcpu hotplug work for qemu Igor Mammedov
2012-01-17 13:17 ` [Qemu-devel] [PATCH 1/3] Introduce a new bus "ICC" to connect APIC Igor Mammedov
2012-01-17 13:17 ` [Qemu-devel] [PATCH 2/3] VCPU hotplug support Igor Mammedov
@ 2012-01-17 13:17 ` Igor Mammedov
2012-01-17 14:18 ` Jan Kiszka
2 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2012-01-17 13:17 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, pingfank, gleb
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
qapi-schema.json | 9 +++++++++
qmp-commands.hx | 26 ++++++++++++++++++++++++++
qmp.c | 15 +++++++++++++++
3 files changed, 50 insertions(+), 0 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 44cf764..05cc582 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -903,6 +903,15 @@
{ 'command': 'cpu', 'data': {'index': 'int'} }
##
+# @cpu_set
+#
+# Sets specified cpu to online/ofline mode
+#
+# Notes: semantics is : cpu_set x online|offline
+##
+{ 'command': 'cpu_set', 'data': {'cpu_index': 'int', 'status': 'str'} }
+
+##
# @memsave:
#
# Save a portion of guest memory to a file.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7e3f4b9..ef1ac1e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -348,6 +348,32 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
EQMP
{
+ .name = "cpu_set",
+ .args_type = "cpu_index:i,status:s",
+ .mhandler.cmd_new = qmp_marshal_input_cpu_set,
+ },
+
+SQMP
+cpu_set
+-------
+
+Sets virtual cpu to online/ofline state
+
+Arguments:
+
+- "cpu_index": virtual cpu index (json-int)
+- "status": desired state of cpu, online/offline (json-string)
+
+Example:
+
+-> { "execute": "cpu_set",
+ "arguments": { "cpu_index": 2,
+ "status": "online" } }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "memsave",
.args_type = "val:l,size:i,filename:s,cpu:i?",
.mhandler.cmd_new = qmp_marshal_input_memsave,
diff --git a/qmp.c b/qmp.c
index c74dde6..e2b268d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -101,6 +101,21 @@ void qmp_cpu(int64_t index, Error **errp)
/* Just do nothing */
}
+void qmp_cpu_set(int64_t cpu_index, const char *status, Error **errp)
+{
+ int state;
+
+ if (!strcmp(status, "online")) {
+ state = 1;
+ } else if (!strcmp(status, "offline")) {
+ state = 0;
+ } else {
+ error_set(errp, QERR_INVALID_PARAMETER, status);
+ return;
+ }
+ qemu_system_cpu_hot_add(cpu_index, state);
+}
+
#ifndef CONFIG_VNC
/* If VNC support is enabled, the "true" query-vnc command is
defined in the VNC subsystem */
--
1.7.7.5
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] add cpu_set qmp command
2012-01-17 13:17 ` [Qemu-devel] [PATCH 3/3] add cpu_set qmp command Igor Mammedov
@ 2012-01-17 14:18 ` Jan Kiszka
2012-01-19 9:38 ` Igor Mammedov
0 siblings, 1 reply; 15+ messages in thread
From: Jan Kiszka @ 2012-01-17 14:18 UTC (permalink / raw)
To: Igor Mammedov
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com
On 2012-01-17 14:17, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> qapi-schema.json | 9 +++++++++
> qmp-commands.hx | 26 ++++++++++++++++++++++++++
> qmp.c | 15 +++++++++++++++
> 3 files changed, 50 insertions(+), 0 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 44cf764..05cc582 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -903,6 +903,15 @@
> { 'command': 'cpu', 'data': {'index': 'int'} }
>
> ##
> +# @cpu_set
> +#
> +# Sets specified cpu to online/ofline mode
> +#
> +# Notes: semantics is : cpu_set x online|offline
> +##
> +{ 'command': 'cpu_set', 'data': {'cpu_index': 'int', 'status': 'str'} }
> +
> +##
> # @memsave:
> #
> # Save a portion of guest memory to a file.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7e3f4b9..ef1ac1e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -348,6 +348,32 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
> EQMP
>
> {
> + .name = "cpu_set",
> + .args_type = "cpu_index:i,status:s",
> + .mhandler.cmd_new = qmp_marshal_input_cpu_set,
> + },
> +
> +SQMP
> +cpu_set
> +-------
> +
> +Sets virtual cpu to online/ofline state
> +
> +Arguments:
> +
> +- "cpu_index": virtual cpu index (json-int)
> +- "status": desired state of cpu, online/offline (json-string)
> +
> +Example:
> +
> +-> { "execute": "cpu_set",
> + "arguments": { "cpu_index": 2,
> + "status": "online" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> + {
> .name = "memsave",
> .args_type = "val:l,size:i,filename:s,cpu:i?",
> .mhandler.cmd_new = qmp_marshal_input_memsave,
> diff --git a/qmp.c b/qmp.c
> index c74dde6..e2b268d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -101,6 +101,21 @@ void qmp_cpu(int64_t index, Error **errp)
> /* Just do nothing */
> }
>
> +void qmp_cpu_set(int64_t cpu_index, const char *status, Error **errp)
> +{
> + int state;
> +
> + if (!strcmp(status, "online")) {
> + state = 1;
> + } else if (!strcmp(status, "offline")) {
> + state = 0;
> + } else {
> + error_set(errp, QERR_INVALID_PARAMETER, status);
> + return;
> + }
> + qemu_system_cpu_hot_add(cpu_index, state);
> +}
> +
> #ifndef CONFIG_VNC
> /* If VNC support is enabled, the "true" query-vnc command is
> defined in the VNC subsystem */
This shouldn't go upstream. We rather need qdev'ified CPUs that can be
added and removed as any other device.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] add cpu_set qmp command
2012-01-17 14:18 ` Jan Kiszka
@ 2012-01-19 9:38 ` Igor Mammedov
2012-01-19 10:24 ` Jan Kiszka
0 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2012-01-19 9:38 UTC (permalink / raw)
To: Jan Kiszka
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com
On 01/17/2012 03:18 PM, Jan Kiszka wrote:
> On 2012-01-17 14:17, Igor Mammedov wrote:
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> ---
>> qapi-schema.json | 9 +++++++++
>> qmp-commands.hx | 26 ++++++++++++++++++++++++++
>> qmp.c | 15 +++++++++++++++
>> 3 files changed, 50 insertions(+), 0 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 44cf764..05cc582 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -903,6 +903,15 @@
>> { 'command': 'cpu', 'data': {'index': 'int'} }
>>
>> ##
>> +# @cpu_set
>> +#
>> +# Sets specified cpu to online/ofline mode
>> +#
>> +# Notes: semantics is : cpu_set x online|offline
>> +##
>> +{ 'command': 'cpu_set', 'data': {'cpu_index': 'int', 'status': 'str'} }
>> +
>> +##
>> # @memsave:
>> #
>> # Save a portion of guest memory to a file.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 7e3f4b9..ef1ac1e 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -348,6 +348,32 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
>> EQMP
>>
>> {
>> + .name = "cpu_set",
>> + .args_type = "cpu_index:i,status:s",
>> + .mhandler.cmd_new = qmp_marshal_input_cpu_set,
>> + },
>> +
>> +SQMP
>> +cpu_set
>> +-------
>> +
>> +Sets virtual cpu to online/ofline state
>> +
>> +Arguments:
>> +
>> +- "cpu_index": virtual cpu index (json-int)
>> +- "status": desired state of cpu, online/offline (json-string)
>> +
>> +Example:
>> +
>> +-> { "execute": "cpu_set",
>> + "arguments": { "cpu_index": 2,
>> + "status": "online" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> + {
>> .name = "memsave",
>> .args_type = "val:l,size:i,filename:s,cpu:i?",
>> .mhandler.cmd_new = qmp_marshal_input_memsave,
>> diff --git a/qmp.c b/qmp.c
>> index c74dde6..e2b268d 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -101,6 +101,21 @@ void qmp_cpu(int64_t index, Error **errp)
>> /* Just do nothing */
>> }
>>
>> +void qmp_cpu_set(int64_t cpu_index, const char *status, Error **errp)
>> +{
>> + int state;
>> +
>> + if (!strcmp(status, "online")) {
>> + state = 1;
>> + } else if (!strcmp(status, "offline")) {
>> + state = 0;
>> + } else {
>> + error_set(errp, QERR_INVALID_PARAMETER, status);
>> + return;
>> + }
>> + qemu_system_cpu_hot_add(cpu_index, state);
>> +}
>> +
>> #ifndef CONFIG_VNC
>> /* If VNC support is enabled, the "true" query-vnc command is
>> defined in the VNC subsystem */
>
> This shouldn't go upstream. We rather need qdev'ified CPUs that can be
> added and removed as any other device.
>
Jan,
Thanks for review!
Then I'll drop this patch and re-post the other ones after fixing them.
> Jan
>
--
Thanks,
Igor
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] add cpu_set qmp command
2012-01-19 9:38 ` Igor Mammedov
@ 2012-01-19 10:24 ` Jan Kiszka
2012-01-19 11:04 ` Igor Mammedov
0 siblings, 1 reply; 15+ messages in thread
From: Jan Kiszka @ 2012-01-19 10:24 UTC (permalink / raw)
To: Igor Mammedov
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com
On 2012-01-19 10:38, Igor Mammedov wrote:
> On 01/17/2012 03:18 PM, Jan Kiszka wrote:
>> On 2012-01-17 14:17, Igor Mammedov wrote:
>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>> ---
>>> qapi-schema.json | 9 +++++++++
>>> qmp-commands.hx | 26 ++++++++++++++++++++++++++
>>> qmp.c | 15 +++++++++++++++
>>> 3 files changed, 50 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 44cf764..05cc582 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -903,6 +903,15 @@
>>> { 'command': 'cpu', 'data': {'index': 'int'} }
>>>
>>> ##
>>> +# @cpu_set
>>> +#
>>> +# Sets specified cpu to online/ofline mode
>>> +#
>>> +# Notes: semantics is : cpu_set x online|offline
>>> +##
>>> +{ 'command': 'cpu_set', 'data': {'cpu_index': 'int', 'status': 'str'} }
>>> +
>>> +##
>>> # @memsave:
>>> #
>>> # Save a portion of guest memory to a file.
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index 7e3f4b9..ef1ac1e 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -348,6 +348,32 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
>>> EQMP
>>>
>>> {
>>> + .name = "cpu_set",
>>> + .args_type = "cpu_index:i,status:s",
>>> + .mhandler.cmd_new = qmp_marshal_input_cpu_set,
>>> + },
>>> +
>>> +SQMP
>>> +cpu_set
>>> +-------
>>> +
>>> +Sets virtual cpu to online/ofline state
>>> +
>>> +Arguments:
>>> +
>>> +- "cpu_index": virtual cpu index (json-int)
>>> +- "status": desired state of cpu, online/offline (json-string)
>>> +
>>> +Example:
>>> +
>>> +-> { "execute": "cpu_set",
>>> + "arguments": { "cpu_index": 2,
>>> + "status": "online" } }
>>> +<- { "return": {} }
>>> +
>>> +EQMP
>>> +
>>> + {
>>> .name = "memsave",
>>> .args_type = "val:l,size:i,filename:s,cpu:i?",
>>> .mhandler.cmd_new = qmp_marshal_input_memsave,
>>> diff --git a/qmp.c b/qmp.c
>>> index c74dde6..e2b268d 100644
>>> --- a/qmp.c
>>> +++ b/qmp.c
>>> @@ -101,6 +101,21 @@ void qmp_cpu(int64_t index, Error **errp)
>>> /* Just do nothing */
>>> }
>>>
>>> +void qmp_cpu_set(int64_t cpu_index, const char *status, Error **errp)
>>> +{
>>> + int state;
>>> +
>>> + if (!strcmp(status, "online")) {
>>> + state = 1;
>>> + } else if (!strcmp(status, "offline")) {
>>> + state = 0;
>>> + } else {
>>> + error_set(errp, QERR_INVALID_PARAMETER, status);
>>> + return;
>>> + }
>>> + qemu_system_cpu_hot_add(cpu_index, state);
>>> +}
>>> +
>>> #ifndef CONFIG_VNC
>>> /* If VNC support is enabled, the "true" query-vnc command is
>>> defined in the VNC subsystem */
>>
>> This shouldn't go upstream. We rather need qdev'ified CPUs that can be
>> added and removed as any other device.
>>
>
> Jan,
>
> Thanks for review!
> Then I'll drop this patch and re-post the other ones after fixing them.
Well, you will still need the control logic of this patch, thus need to
think about converting x86 CPUs to qdev. Otherwise, testing will only be
possible over qemu-kvm.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] add cpu_set qmp command
2012-01-19 10:24 ` Jan Kiszka
@ 2012-01-19 11:04 ` Igor Mammedov
0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2012-01-19 11:04 UTC (permalink / raw)
To: Jan Kiszka
Cc: pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org,
gleb@redhat.com
On 01/19/2012 11:24 AM, Jan Kiszka wrote:
> On 2012-01-19 10:38, Igor Mammedov wrote:
>> On 01/17/2012 03:18 PM, Jan Kiszka wrote:
>>> On 2012-01-17 14:17, Igor Mammedov wrote:
>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>> ---
>>>> qapi-schema.json | 9 +++++++++
>>>> qmp-commands.hx | 26 ++++++++++++++++++++++++++
>>>> qmp.c | 15 +++++++++++++++
>>>> 3 files changed, 50 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index 44cf764..05cc582 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -903,6 +903,15 @@
>>>> { 'command': 'cpu', 'data': {'index': 'int'} }
>>>>
>>>> ##
>>>> +# @cpu_set
>>>> +#
>>>> +# Sets specified cpu to online/ofline mode
>>>> +#
>>>> +# Notes: semantics is : cpu_set x online|offline
>>>> +##
>>>> +{ 'command': 'cpu_set', 'data': {'cpu_index': 'int', 'status': 'str'} }
>>>> +
>>>> +##
>>>> # @memsave:
>>>> #
>>>> # Save a portion of guest memory to a file.
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index 7e3f4b9..ef1ac1e 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -348,6 +348,32 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
>>>> EQMP
>>>>
>>>> {
>>>> + .name = "cpu_set",
>>>> + .args_type = "cpu_index:i,status:s",
>>>> + .mhandler.cmd_new = qmp_marshal_input_cpu_set,
>>>> + },
>>>> +
>>>> +SQMP
>>>> +cpu_set
>>>> +-------
>>>> +
>>>> +Sets virtual cpu to online/ofline state
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "cpu_index": virtual cpu index (json-int)
>>>> +- "status": desired state of cpu, online/offline (json-string)
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "cpu_set",
>>>> + "arguments": { "cpu_index": 2,
>>>> + "status": "online" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> + {
>>>> .name = "memsave",
>>>> .args_type = "val:l,size:i,filename:s,cpu:i?",
>>>> .mhandler.cmd_new = qmp_marshal_input_memsave,
>>>> diff --git a/qmp.c b/qmp.c
>>>> index c74dde6..e2b268d 100644
>>>> --- a/qmp.c
>>>> +++ b/qmp.c
>>>> @@ -101,6 +101,21 @@ void qmp_cpu(int64_t index, Error **errp)
>>>> /* Just do nothing */
>>>> }
>>>>
>>>> +void qmp_cpu_set(int64_t cpu_index, const char *status, Error **errp)
>>>> +{
>>>> + int state;
>>>> +
>>>> + if (!strcmp(status, "online")) {
>>>> + state = 1;
>>>> + } else if (!strcmp(status, "offline")) {
>>>> + state = 0;
>>>> + } else {
>>>> + error_set(errp, QERR_INVALID_PARAMETER, status);
>>>> + return;
>>>> + }
>>>> + qemu_system_cpu_hot_add(cpu_index, state);
>>>> +}
>>>> +
>>>> #ifndef CONFIG_VNC
>>>> /* If VNC support is enabled, the "true" query-vnc command is
>>>> defined in the VNC subsystem */
>>>
>>> This shouldn't go upstream. We rather need qdev'ified CPUs that can be
>>> added and removed as any other device.
>>>
>>
>> Jan,
>>
>> Thanks for review!
>> Then I'll drop this patch and re-post the other ones after fixing them.
>
> Well, you will still need the control logic of this patch, thus need to
> think about converting x86 CPUs to qdev. Otherwise, testing will only be
> possible over qemu-kvm.
>
> Jan
>
I've agree that converting x86 CPUs to qdev is needed. I've seen RFC
patches for qev-ifying ppc CPU but they haven't made to upstream yet. I'll
look at them as model for x86 CPU conversion but I can't promise fast
results here (I'm complete newbie in this). I'm open to suggestions if
you have a better idea how to do it or a better example.
I think there is sense in having in the tree the first 2 patches separately
from qdev-ifying cpu patches, since they provide a basic infrastructure
for vcpu hot-plug and we will reduce difference between qemu and qemu-kvm
in this parts of code. In addition, it will open a road for hot-unplug
patches that people send now against qemu-kvm.
It still will be possible to play with hot-plug using 3rd patch off the tree
while qdev-ifed cpu patch(es) in works.
--
Thanks,
Igor
^ permalink raw reply [flat|nested] 15+ messages in thread