[libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device. So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'. The cpu device's xml like this: <cpu driver='qemu64-x86_64-cpu' apic_id='3'> Zhu Guihua (12): domain_conf: allocate cpu's apic id dynamically domain_conf: add support for cpu device configuration in XML domain_conf: introduce cpu def helpers domain_conf: introduce cpu device hotplug helpers qemu_driver: implement cpu device hotplug on config level qemu_command: introduce a func for cpu device alias assignment qemu: implement support for qemu64-x86_64-cpu qemu: introduce qemuBuildCPUDeviceStr qemu: implement cpu device hotplug on live level qemu: implement cpu device hotunplug on live level qemu_monitor_json: sort JSON array of cpu info qemu_driver: detect threads corresponding to Vcpus src/conf/domain_conf.c | 188 +++++++++++++++++++++++++++ src/conf/domain_conf.h | 35 +++++ src/libvirt_private.syms | 6 + src/qemu/qemu_capabilities.c | 3 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 83 +++++++++++- src/qemu/qemu_command.h | 8 ++ src/qemu/qemu_driver.c | 296 ++++++++++++++++++++++++------------------- src/qemu/qemu_driver.h | 8 ++ src/qemu/qemu_hotplug.c | 129 +++++++++++++++++++ src/qemu/qemu_hotplug.h | 11 ++ src/qemu/qemu_monitor_json.c | 31 ++++- src/util/virbitmap.c | 2 +- src/util/virbitmap.h | 2 + 14 files changed, 668 insertions(+), 135 deletions(-) -- 1.9.3
Add a bitmap apic_idmap to store status of APIC IDs. If you want to use an APIC ID, you can find a minimum value which has not been used in the bitmap. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 15 +++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 5 ++++- 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8792f5e..1631421 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12895,6 +12895,12 @@ virDomainDefParseXML(xmlDocPtr xml, } } + if (!(def->apic_id_map = virBitmapNew(def->maxvcpus))) + goto error; + + for (i = 0; i < def->vcpus; i++) + ignore_value(virBitmapSetBit(def->apic_id_map, i)); + tmp = virXPathString("string(./vcpu[1]/@placement)", ctxt); if (tmp) { if ((def->placement_mode = @@ -16288,6 +16294,15 @@ virDomainVcpuPinDel(virDomainDefPtr def, int vcpu) } } +uint32_t +virDomainCPUGetFreeApicID(virDomainDefPtr def) +{ + int i; + i = virBitmapNextClearBit(def->apic_id_map, 0); + + return i; +} + int virDomainEmulatorPinAdd(virDomainDefPtr def, unsigned char *cpumap, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 09ab194..8869d26 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2062,6 +2062,7 @@ struct _virDomainDef { unsigned short maxvcpus; int placement_mode; virBitmapPtr cpumask; + virBitmapPtr apic_id_map; unsigned int iothreads; @@ -2530,6 +2531,8 @@ int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, void virDomainVcpuPinDel(virDomainDefPtr def, int vcpu); +uint32_t virDomainCPUGetFreeApicID(virDomainDefPtr def); + int virDomainEmulatorPinAdd(virDomainDefPtr def, unsigned char *cpumap, int maplen); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a2eec83..d08e400 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -426,6 +426,7 @@ virDomainVcpuPinDefFree; virDomainVcpuPinDel; virDomainVcpuPinFindByVcpu; virDomainVcpuPinIsDuplicate; +virDomainCPUGetFreeApicID; virDomainVideoDefaultRAM; virDomainVideoDefaultType; virDomainVideoDefFree; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5994558..6bc7d8d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4371,6 +4371,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, int ncpupids; virCgroupPtr cgroup_vcpu = NULL; char *mem_mask = NULL; + uint32_t apic_id; qemuDomainObjEnterMonitor(driver, vm); @@ -4380,13 +4381,15 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, if (nvcpus > vcpus) { for (i = vcpus; i < nvcpus; i++) { /* Online new CPU */ - rc = qemuMonitorSetCPU(priv->mon, i, true); + apic_id = virDomainCPUGetFreeApicID(vm->def); + rc = qemuMonitorSetCPU(priv->mon, apic_id, true); if (rc == 0) goto unsupported; if (rc < 0) goto exit_monitor; vcpus++; + ignore_value(virBitmapSetBit(vm->def->apic_id_map, apic_id)); } } else { for (i = vcpus - 1; i >= nvcpus; i--) { -- 1.9.3
On Wed, Jan 21, 2015 at 16:00:53 +0800, Zhu Guihua wrote:
Add a bitmap apic_idmap to store status of APIC IDs. If you want to use an APIC ID, you can find a minimum value which has not been used in the bitmap.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 15 +++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 5 ++++- 4 files changed, 23 insertions(+), 1 deletion(-)
...
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a2eec83..d08e400 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -426,6 +426,7 @@ virDomainVcpuPinDefFree; virDomainVcpuPinDel; virDomainVcpuPinFindByVcpu; virDomainVcpuPinIsDuplicate; +virDomainCPUGetFreeApicID; virDomainVideoDefaultRAM; virDomainVideoDefaultType; virDomainVideoDefFree;
make syntax-check enforces that this file is ordered by file and then by function name. Please order this hunk correctly and run make syntax-check before the submission. Peter
On Wed, 2015-01-21 at 09:56 +0100, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 16:00:53 +0800, Zhu Guihua wrote:
Add a bitmap apic_idmap to store status of APIC IDs. If you want to use an APIC ID, you can find a minimum value which has not been used in the bitmap.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 15 +++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 5 ++++- 4 files changed, 23 insertions(+), 1 deletion(-)
...
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a2eec83..d08e400 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -426,6 +426,7 @@ virDomainVcpuPinDefFree; virDomainVcpuPinDel; virDomainVcpuPinFindByVcpu; virDomainVcpuPinIsDuplicate; +virDomainCPUGetFreeApicID; virDomainVideoDefaultRAM; virDomainVideoDefaultType; virDomainVideoDefFree;
make syntax-check enforces that this file is ordered by file and then by function name. Please order this hunk correctly and run make syntax-check before the submission.
Got it, thanks. Regards, Zhu
Peter
This patch adds configuration support for the cpu device. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 14 ++++++++++++++ src/conf/domain_conf.h | 19 +++++++++++++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_hotplug.c | 1 + 5 files changed, 41 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1631421..e036d75 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -231,6 +231,7 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "redirdev", "smartcard", "chr", + "cpu", "memballoon", "nvram", "rng", @@ -1981,6 +1982,7 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) case VIR_DOMAIN_DEVICE_PANIC: virDomainPanicDefFree(def->data.panic); break; + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_NONE: break; @@ -2675,6 +2677,8 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device) return &device->data.smartcard->info; case VIR_DOMAIN_DEVICE_CHR: return &device->data.chr->info; + case VIR_DOMAIN_DEVICE_CPU: + return &device->data.cpu->info; case VIR_DOMAIN_DEVICE_MEMBALLOON: return &device->data.memballoon->info; case VIR_DOMAIN_DEVICE_NVRAM: @@ -2857,6 +2861,12 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, if (cb(def, &device, &def->consoles[i]->info, opaque) < 0) return -1; } + device.type = VIR_DOMAIN_DEVICE_CPU; + for (i = 0; i < def->ncpus; i++) { + device.data.cpu = def->cpus[i]; + if (cb(def, &device, &def->cpus[i]->info, opaque) < 0) + return -1; + } device.type = VIR_DOMAIN_DEVICE_INPUT; for (i = 0; i < def->ninputs; i++) { device.data.input = def->inputs[i]; @@ -2943,6 +2953,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_SHMEM: @@ -11175,6 +11186,7 @@ virDomainDeviceDefParse(const char *xmlStr, if (!(dev->data.panic = virDomainPanicDefParseXML(node))) goto error; break; + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LAST: break; @@ -16015,6 +16027,7 @@ virDomainDefCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_LAST: @@ -21463,6 +21476,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, case VIR_DOMAIN_DEVICE_PANIC: rc = virDomainPanicDefFormat(&buf, src->data.panic); break; + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8869d26..618eef3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -108,6 +108,9 @@ typedef virDomainSmartcardDef *virDomainSmartcardDefPtr; typedef struct _virDomainChrDef virDomainChrDef; typedef virDomainChrDef *virDomainChrDefPtr; +typedef struct _virDomainCPUDef virDomainCPUDef; +typedef virDomainCPUDef *virDomainCPUDefPtr; + typedef struct _virDomainMemballoonDef virDomainMemballoonDef; typedef virDomainMemballoonDef *virDomainMemballoonDefPtr; @@ -162,6 +165,7 @@ typedef enum { VIR_DOMAIN_DEVICE_REDIRDEV, VIR_DOMAIN_DEVICE_SMARTCARD, VIR_DOMAIN_DEVICE_CHR, + VIR_DOMAIN_DEVICE_CPU, VIR_DOMAIN_DEVICE_MEMBALLOON, VIR_DOMAIN_DEVICE_NVRAM, VIR_DOMAIN_DEVICE_RNG, @@ -192,6 +196,7 @@ struct _virDomainDeviceDef { virDomainRedirdevDefPtr redirdev; virDomainSmartcardDefPtr smartcard; virDomainChrDefPtr chr; + virDomainCPUDefPtr cpu; virDomainMemballoonDefPtr memballoon; virDomainNVRAMDefPtr nvram; virDomainRNGDefPtr rng; @@ -1158,6 +1163,13 @@ struct _virDomainChrDef { virSecurityDeviceLabelDefPtr *seclabels; }; +struct _virDomainCPUDef { + char *driver; + int apic_id; + + virDomainDeviceInfo info; +}; + typedef enum { VIR_DOMAIN_SMARTCARD_TYPE_HOST, VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES, @@ -2130,6 +2142,9 @@ struct _virDomainDef { size_t nsmartcards; virDomainSmartcardDefPtr *smartcards; + size_t ncpus; + virDomainCPUDefPtr *cpus; + size_t nserials; virDomainChrDefPtr *serials; @@ -2342,6 +2357,7 @@ void virDomainActualNetDefFree(virDomainActualNetDefPtr def); void virDomainNetDefFree(virDomainNetDefPtr def); void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def); void virDomainChrDefFree(virDomainChrDefPtr def); +void virDomainCPUDefFree(virDomainCPUDefPtr def); void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def); int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr src, virDomainChrSourceDefPtr dest); @@ -2387,6 +2403,8 @@ void virDomainDefFree(virDomainDefPtr vm); virDomainChrDefPtr virDomainChrDefNew(void); +virDomainCPUDefPtr virDomainCPUDefNew(void); + virDomainDefPtr virDomainDefNew(const char *name, const unsigned char *uuid, int id); @@ -2805,6 +2823,7 @@ VIR_ENUM_DECL(virDomainChrChannelTarget) VIR_ENUM_DECL(virDomainChrConsoleTarget) VIR_ENUM_DECL(virDomainChrSerialTarget) VIR_ENUM_DECL(virDomainSmartcard) +VIR_ENUM_DECL(virDomainCPU) VIR_ENUM_DECL(virDomainChr) VIR_ENUM_DECL(virDomainChrTcpProtocol) VIR_ENUM_DECL(virDomainChrSpicevmc) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d08e400..9ceff71 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -166,6 +166,7 @@ virDomainChrTcpProtocolTypeFromString; virDomainChrTcpProtocolTypeToString; virDomainChrTypeFromString; virDomainChrTypeToString; +virDomainCPUDefFree; virDomainClockBasisTypeToString; virDomainClockOffsetTypeFromString; virDomainClockOffsetTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6bc7d8d..ecdf5c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6994,6 +6994,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, dev->data.chr = NULL; break; + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -7069,6 +7070,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: @@ -7188,6 +7190,7 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: @@ -7312,6 +7315,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, dev->data.fs = NULL; break; + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: @@ -7428,6 +7432,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainFSDefFree(fs); break; + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: @@ -7543,6 +7548,7 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9ed96dc..f6d7667 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2885,6 +2885,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: -- 1.9.3
On Wed, Jan 21, 2015 at 16:00:54 +0800, Zhu Guihua wrote:
This patch adds configuration support for the cpu device.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 14 ++++++++++++++ src/conf/domain_conf.h | 19 +++++++++++++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_hotplug.c | 1 + 5 files changed, 41 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1631421..e036d75 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
@@ -2857,6 +2861,12 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, if (cb(def, &device, &def->consoles[i]->info, opaque) < 0) return -1; } + device.type = VIR_DOMAIN_DEVICE_CPU; + for (i = 0; i < def->ncpus; i++) { + device.data.cpu = def->cpus[i]; + if (cb(def, &device, &def->cpus[i]->info, opaque) < 0) + return -1; + } device.type = VIR_DOMAIN_DEVICE_INPUT; for (i = 0; i < def->ninputs; i++) { device.data.input = def->inputs[i];
@@ -16015,6 +16027,7 @@ virDomainDefCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_CPU: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_LAST:
The ABI stability check code needs to be implemented.
@@ -21463,6 +21476,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, case VIR_DOMAIN_DEVICE_PANIC: rc = virDomainPanicDefFormat(&buf, src->data.panic); break; + case VIR_DOMAIN_DEVICE_CPU:
The copy function needs to be impelemnted too (I didn't check next patch thoug yet)
case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8869d26..618eef3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -108,6 +108,9 @@ typedef virDomainSmartcardDef *virDomainSmartcardDefPtr; typedef struct _virDomainChrDef virDomainChrDef; typedef virDomainChrDef *virDomainChrDefPtr;
+typedef struct _virDomainCPUDef virDomainCPUDef; +typedef virDomainCPUDef *virDomainCPUDefPtr; + typedef struct _virDomainMemballoonDef virDomainMemballoonDef; typedef virDomainMemballoonDef *virDomainMemballoonDefPtr;
@@ -1158,6 +1163,13 @@ struct _virDomainChrDef {
virSecurityDeviceLabelDefPtr *seclabels; };
+struct _virDomainCPUDef { + char *driver; + int apic_id; + + virDomainDeviceInfo info;
I've looked through the next patch and you don't parse any device info for the CPU type. Is it necessary to have it present at all in that case?
+}; + typedef enum { VIR_DOMAIN_SMARTCARD_TYPE_HOST, VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES,
@@ -2342,6 +2357,7 @@ void virDomainActualNetDefFree(virDomainActualNetDefPtr def); void virDomainNetDefFree(virDomainNetDefPtr def); void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def); void virDomainChrDefFree(virDomainChrDefPtr def); +void virDomainCPUDefFree(virDomainCPUDefPtr def);
Function declarations should be along with function definitions.
void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def); int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr src, virDomainChrSourceDefPtr dest); @@ -2387,6 +2403,8 @@ void virDomainDefFree(virDomainDefPtr vm);
virDomainChrDefPtr virDomainChrDefNew(void);
+virDomainCPUDefPtr virDomainCPUDefNew(void); +
Same here
virDomainDefPtr virDomainDefNew(const char *name, const unsigned char *uuid, int id); @@ -2805,6 +2823,7 @@ VIR_ENUM_DECL(virDomainChrChannelTarget) VIR_ENUM_DECL(virDomainChrConsoleTarget) VIR_ENUM_DECL(virDomainChrSerialTarget) VIR_ENUM_DECL(virDomainSmartcard) +VIR_ENUM_DECL(virDomainCPU) VIR_ENUM_DECL(virDomainChr) VIR_ENUM_DECL(virDomainChrTcpProtocol) VIR_ENUM_DECL(virDomainChrSpicevmc) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d08e400..9ceff71 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -166,6 +166,7 @@ virDomainChrTcpProtocolTypeFromString; virDomainChrTcpProtocolTypeToString; virDomainChrTypeFromString; virDomainChrTypeToString; +virDomainCPUDefFree;
Breaks "make syntax-check" again: Expected symbol virDomainCPUDefFree is not in ELF library The symbols entry can be added only when the function is defined.
virDomainClockBasisTypeToString; virDomainClockOffsetTypeFromString; virDomainClockOffsetTypeToString;
Peter
virDomainCPUDefFree - free memory allocated virDomainCPUDefParseXML - parse job type virDomainCPUDefFormat - output job type Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e036d75..1f05056 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1913,6 +1913,19 @@ void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def) VIR_FREE(def); } +void virDomainCPUDefFree(virDomainCPUDefPtr def) +{ + if (!def) + return; + + if (def->driver) + VIR_FREE(def->driver); + + virDomainDeviceInfoClear(&def->info); + + VIR_FREE(def); +} + void virDomainDeviceDefFree(virDomainDeviceDefPtr def) { if (!def) @@ -1983,6 +1996,8 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) virDomainPanicDefFree(def->data.panic); break; case VIR_DOMAIN_DEVICE_CPU: + virDomainCPUDefFree(def->data.cpu); + break; case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_NONE: break; @@ -11052,6 +11067,57 @@ virDomainPMStateParseXML(xmlXPathContextPtr ctxt, return ret; } +virDomainCPUDefPtr +virDomainCPUDefNew(void) +{ + virDomainCPUDefPtr def = NULL; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + +static virDomainCPUDefPtr +virDomainCPUDefParseXML(xmlNodePtr node, virDomainDefPtr def) +{ + char *driver = NULL; + char *apic_id = NULL; + virDomainCPUDefPtr dev; + + if (!(dev = virDomainCPUDefNew())) + return NULL; + + driver = virXMLPropString(node, "driver"); + if (driver == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing cpu device driver")); + goto error; + } + dev->driver = driver; + + apic_id = virXMLPropString(node, "apic_id"); + + if (!apic_id) + dev->apic_id = virDomainCPUGetFreeApicID(def); + else + dev->apic_id = atoi (apic_id); + + driver = NULL; + apic_id = NULL; + + cleanup: + VIR_FREE(driver); + VIR_FREE(apic_id); + + return dev; + + error: + virDomainCPUDefFree(dev); + dev = NULL; + goto cleanup; +} + virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, const virDomainDef *def, @@ -11187,6 +11253,9 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_CPU: + if (!(dev->data.cpu = virDomainCPUDefParseXML(node, (virDomainDefPtr)def))) + goto error; + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LAST: break; @@ -18142,6 +18211,31 @@ virDomainChrDefFormat(virBufferPtr buf, } static int +virDomainCPUDefFormat(virBufferPtr buf, + virDomainCPUDefPtr def, + unsigned int flags) +{ + char *apic_id = NULL; + + ignore_value(virAsprintf(&apic_id, "%d", def->apic_id)); + + virBufferAsprintf(buf, "<cpu driver='%s'", def->driver); + + virBufferEscapeString(buf, " apic_id='%s'", apic_id); + + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</cpu>\n"); + + return 0; +} + +static int virDomainSmartcardDefFormat(virBufferPtr buf, virDomainSmartcardDefPtr def, unsigned int flags) @@ -20062,6 +20156,10 @@ virDomainDefFormatInternal(virDomainDefPtr def, goto error; } + for (n = 0; n < def->ncpus; n++) + if (virDomainCPUDefFormat(buf, def->cpus[n], flags) < 0) + goto error; + if (def->nvram) virDomainNVRAMDefFormat(buf, def->nvram, flags); @@ -21477,6 +21575,8 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, rc = virDomainPanicDefFormat(&buf, src->data.panic); break; case VIR_DOMAIN_DEVICE_CPU: + rc = virDomainCPUDefFormat(&buf, src->data.cpu, flags); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: -- 1.9.3
On Wed, Jan 21, 2015 at 16:00:55 +0800, Zhu Guihua wrote:
virDomainCPUDefFree - free memory allocated virDomainCPUDefParseXML - parse job type virDomainCPUDefFormat - output job type
This patch lacks addition to the RNG schemas that would describe the elements that are added and the correct values. Also lacks change to the docs/formatdomain.html.in
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e036d75..1f05056 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
...
+ +static virDomainCPUDefPtr +virDomainCPUDefParseXML(xmlNodePtr node, virDomainDefPtr def) +{ + char *driver = NULL; + char *apic_id = NULL; + virDomainCPUDefPtr dev; + + if (!(dev = virDomainCPUDefNew())) + return NULL; + + driver = virXMLPropString(node, "driver");
The driver should rather be an enum value that is parsed from the string rather than stroing an inline string. This limits the namespace of devices libvirt supports but simplifies all matching of the driver later on.
+ if (driver == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing cpu device driver")); + goto error; + } + dev->driver = driver; + + apic_id = virXMLPropString(node, "apic_id"); + + if (!apic_id) + dev->apic_id = virDomainCPUGetFreeApicID(def); + else + dev->apic_id = atoi (apic_id);
Atoi is not allowed, use virStrToLong* instead. Also spaces after function name is forbidden.
+ + driver = NULL; + apic_id = NULL; + + cleanup: + VIR_FREE(driver); + VIR_FREE(apic_id); + + return dev; + + error: + virDomainCPUDefFree(dev); + dev = NULL; + goto cleanup; +} + virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, const virDomainDef *def, @@ -11187,6 +11253,9 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_CPU: + if (!(dev->data.cpu = virDomainCPUDefParseXML(node, (virDomainDefPtr)def))) + goto error; + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LAST: break; @@ -18142,6 +18211,31 @@ virDomainChrDefFormat(virBufferPtr buf, }
static int +virDomainCPUDefFormat(virBufferPtr buf, + virDomainCPUDefPtr def, + unsigned int flags) +{ + char *apic_id = NULL; + + ignore_value(virAsprintf(&apic_id, "%d", def->apic_id)); + + virBufferAsprintf(buf, "<cpu driver='%s'", def->driver); + + virBufferEscapeString(buf, " apic_id='%s'", apic_id);
Um? Why not virBufferAsprintf(buf, " apic_id='%d', apic_id)? You won't need the intermediate string. Also it leaks the apic_id string.
+ + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</cpu>\n"); + + return 0; +} + +static int
Peter
On Wed, 2015-01-21 at 10:16 +0100, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 16:00:55 +0800, Zhu Guihua wrote:
virDomainCPUDefFree - free memory allocated virDomainCPUDefParseXML - parse job type virDomainCPUDefFormat - output job type
This patch lacks addition to the RNG schemas that would describe the elements that are added and the correct values.
Also lacks change to the docs/formatdomain.html.in
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e036d75..1f05056 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
...
+ +static virDomainCPUDefPtr +virDomainCPUDefParseXML(xmlNodePtr node, virDomainDefPtr def) +{ + char *driver = NULL; + char *apic_id = NULL; + virDomainCPUDefPtr dev; + + if (!(dev = virDomainCPUDefNew())) + return NULL; + + driver = virXMLPropString(node, "driver");
The driver should rather be an enum value that is parsed from the string rather than stroing an inline string. This limits the namespace of devices libvirt supports but simplifies all matching of the driver later on.
Yes, it will be an enum value in next version, and it will also support more driver.
+ if (driver == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing cpu device driver")); + goto error; + } + dev->driver = driver; + + apic_id = virXMLPropString(node, "apic_id"); + + if (!apic_id) + dev->apic_id = virDomainCPUGetFreeApicID(def); + else + dev->apic_id = atoi (apic_id);
Atoi is not allowed, use virStrToLong* instead. Also spaces after function name is forbidden.
I will change it.
+ + driver = NULL; + apic_id = NULL; + + cleanup: + VIR_FREE(driver); + VIR_FREE(apic_id); + + return dev; + + error: + virDomainCPUDefFree(dev); + dev = NULL; + goto cleanup; +} + virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, const virDomainDef *def, @@ -11187,6 +11253,9 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_CPU: + if (!(dev->data.cpu = virDomainCPUDefParseXML(node, (virDomainDefPtr)def))) + goto error; + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LAST: break; @@ -18142,6 +18211,31 @@ virDomainChrDefFormat(virBufferPtr buf, }
static int +virDomainCPUDefFormat(virBufferPtr buf, + virDomainCPUDefPtr def, + unsigned int flags) +{ + char *apic_id = NULL; + + ignore_value(virAsprintf(&apic_id, "%d", def->apic_id)); + + virBufferAsprintf(buf, "<cpu driver='%s'", def->driver); + + virBufferEscapeString(buf, " apic_id='%s'", apic_id);
Um? Why not virBufferAsprintf(buf, " apic_id='%d', apic_id)?
You won't need the intermediate string. Also it leaks the apic_id string.
Got it.
+ + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</cpu>\n"); + + return 0; +} + +static int
Peter
Regards, Zhu
virDomainCPUFind - to find a cpu within VM def virDomainCPUInsert - wrapper for inserting a new cpu device into VM def virDomainCPURemove - wrapper for removing cpu from VM def Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 13 +++++++++++ src/libvirt_private.syms | 3 +++ 3 files changed, 75 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1f05056..45f954f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12142,6 +12142,65 @@ virDomainChrRemove(virDomainDefPtr vmdef, return ret; } +int +virDomainCPUInsert(virDomainDefPtr vmdef, + virDomainCPUDefPtr cpu) +{ + return VIR_APPEND_ELEMENT(vmdef->cpus, vmdef->ncpus, cpu); +} + +bool +virDomainCPUEquals(virDomainCPUDefPtr src, + virDomainCPUDefPtr tgt) +{ + bool ret = false; + + if (!src || !tgt) + return src == tgt; + + if (src->apic_id == tgt->apic_id) + ret = true; + + return ret; +} + +virDomainCPUDefPtr +virDomainCPUFind(virDomainDefPtr def, + virDomainCPUDefPtr target) +{ + virDomainCPUDefPtr cpu; + size_t i; + + for (i = 0; i < def->ncpus; i++) { + cpu = def->cpus[i]; + if (virDomainCPUEquals(cpu, target)) + return cpu; + } + + return NULL; +} + +virDomainCPUDefPtr +virDomainCPURemove(virDomainDefPtr vmdef, + virDomainCPUDefPtr cpu) +{ + virDomainCPUDefPtr ret; + size_t i; + + for (i = 0; i < vmdef->ncpus; i++) { + ret = vmdef->cpus[i]; + + if (virDomainCPUEquals(ret, cpu)) + break; + } + + if (i == vmdef->ncpus) + return NULL; + + VIR_DELETE_ELEMENT(vmdef->cpus, i, vmdef->ncpus); + return ret; +} + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 618eef3..4b71052 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2674,6 +2674,19 @@ virDomainChrDefPtr virDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr); +int +virDomainCPUInsert(virDomainDefPtr vmdef, + virDomainCPUDefPtr cpu); +bool +virDomainCPUEquals(virDomainCPUDefPtr src, + virDomainCPUDefPtr tgt); +virDomainCPUDefPtr +virDomainCPUFind(virDomainDefPtr def, + virDomainCPUDefPtr cpu); +virDomainCPUDefPtr +virDomainCPURemove(virDomainDefPtr vmdef, + virDomainCPUDefPtr cpu); + int virDomainSaveXML(const char *configDir, virDomainDefPtr def, const char *xml); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9ceff71..032e9a9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -167,6 +167,9 @@ virDomainChrTcpProtocolTypeToString; virDomainChrTypeFromString; virDomainChrTypeToString; virDomainCPUDefFree; +virDomainCPUInsert; +virDomainCPUFind; +virDomainCPURemove; virDomainClockBasisTypeToString; virDomainClockOffsetTypeFromString; virDomainClockOffsetTypeToString; -- 1.9.3
The config level requires an insert or remove from domain definition structure. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ecdf5c6..a88f6b4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7316,6 +7316,11 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, break; case VIR_DOMAIN_DEVICE_CPU: + if (virDomainCPUInsert(vmdef, dev->data.cpu) < 0) + return -1; + dev->data.cpu = NULL; + break; + case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: @@ -7352,6 +7357,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainControllerDefPtr cont, det_cont; virDomainChrDefPtr chr; virDomainFSDefPtr fs; + virDomainCPUDefPtr cpu; int idx; switch ((virDomainDeviceType) dev->type) { @@ -7433,6 +7439,14 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, break; case VIR_DOMAIN_DEVICE_CPU: + if (!(cpu = virDomainCPURemove(vmdef, dev->data.cpu))) + return -1; + + virDomainCPUDefFree(cpu); + virDomainCPUDefFree(dev->data.cpu); + dev->data.cpu = NULL; + break; + case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: -- 1.9.3
This function used to set a alias name for cpu device. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_command.c | 32 ++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 3 +++ 2 files changed, 35 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c041ee7..69d0a2a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -946,6 +946,34 @@ qemuAssignDeviceChrAlias(virDomainDefPtr def, } int +qemuAssignDeviceCPUAlias(virDomainDefPtr def, + virDomainCPUDefPtr cpu, + int idx) +{ + if (idx == -1) { + size_t i; + idx = 0; + + for (i = 0; i < def->ncpus; i++) { + int thisidx; + if ((thisidx = qemuDomainDeviceAliasIndex(&def->cpus[i]->info, "cpu")) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to determine device index from CPU device")); + return -1; + } + + if (thisidx >= idx) + idx = thisidx + 1; + } + } + + if (virAsprintf(&cpu->info.alias, "cpu%d", idx) < 0) + return -1; + + return 0; +} + +int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { size_t i; @@ -1015,6 +1043,10 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0) return -1; } + for (i = 0; i< def->ncpus; i++) { + if (qemuAssignDeviceCPUAlias(def, def->cpus[i], i) < 0) + return -1; + } for (i = 0; i < def->nhubs; i++) { if (virAsprintf(&def->hubs[i]->info.alias, "hub%zu", i) < 0) return -1; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index dcc7127..2898876 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -264,6 +264,9 @@ int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr r int qemuAssignDeviceChrAlias(virDomainDefPtr def, virDomainChrDefPtr chr, ssize_t idx); +int qemuAssignDeviceCPUAlias(virDomainDefPtr def, + virDomainCPUDefPtr cpu, + int idx); int qemuParseKeywords(const char *str, -- 1.9.3
This patch adds a new capability bit QEMU_CAPS_QEMU64_X86_64_CPU. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 1 - 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 13f3cd3..56bb588 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -277,6 +277,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "vmware-svga.vgamem_mb", "qxl.vgamem_mb", "qxl-vga.vgamem_mb", + "qemu64-x86_64-cpu", ); @@ -1524,6 +1525,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "usb-audio", QEMU_CAPS_OBJECT_USB_AUDIO }, { "iothread", QEMU_CAPS_OBJECT_IOTHREAD}, { "ivshmem", QEMU_CAPS_DEVICE_IVSHMEM }, + { "qemu64-x86_64-cpu", QEMU_CAPS_DEVICE_QEMU64_CPU }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { @@ -3157,6 +3159,7 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps, virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_ACPI); virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_HPET); virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT); + virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_QEMU64_CPU); } ret = 0; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 12e1688..479fe4c 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -223,6 +223,7 @@ typedef enum { QEMU_CAPS_VMWARE_SVGA_VGAMEM = 181, /* -device vmware-svga.vgamem_mb */ QEMU_CAPS_QXL_VGAMEM = 182, /* -device qxl.vgamem_mb */ QEMU_CAPS_QXL_VGA_VGAMEM = 183, /* -device qxl-vga.vgamem_mb */ + QEMU_CAPS_DEVICE_QEMU64_CPU = 184, /* -device qemu64-x86_64-cpu */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 69d0a2a..2ee3e10 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5876,7 +5876,6 @@ qemuBuildRNGDeviceArgs(virCommandPtr cmd, return ret; } - static char *qemuBuildTPMBackendStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps, const char *emulator) -- 1.9.3
On Wed, Jan 21, 2015 at 16:00:59 +0800, Zhu Guihua wrote:
This patch adds a new capability bit QEMU_CAPS_QEMU64_X86_64_CPU.
Misleading subject. This patch adds the capability, but does not impelment support for the device.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 1 - 3 files changed, 4 insertions(+), 1 deletion(-)
...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 69d0a2a..2ee3e10 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5876,7 +5876,6 @@ qemuBuildRNGDeviceArgs(virCommandPtr cmd, return ret; }
-
Spurious whitespace change.
static char *qemuBuildTPMBackendStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps, const char *emulator)
Peter
qemuBuildCPUDeviceStr being introduced is responsible for creating command line argument for '-device' for given cpu device. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_command.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 5 +++++ 2 files changed, 55 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2ee3e10..824ad29 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5956,6 +5956,50 @@ static char *qemuBuildTPMDevStr(const virDomainDef *def, } +int +qemuBuildCPUDeviceStr(char **deviceStr, + virDomainCPUDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU64_CPU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s not supported in this QEMU binary"), dev->driver); + goto error; + } + + virBufferAsprintf(&buf, "%s,id=%s,apic-id=%d", + dev->driver, dev->info.alias, + dev->apic_id); + + if (virBufferCheckError(&buf) < 0) + goto error; + + *deviceStr = virBufferContentAndReset(&buf); + return 0; + + error: + virBufferFreeAndReset(&buf); + return -1; +} + +static int +qemuBuildCPUDeviceCommandLine(virCommandPtr cmd, + virDomainCPUDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + char *devstr = NULL; + + if (qemuBuildCPUDeviceStr(&devstr, dev, qemuCaps) < 0) + return -1; + + virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); + return 0; +} + + static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -9862,6 +9906,12 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } + /* add cpu devices */ + for (i = 0; i < def->ncpus; i++) { + if (qemuBuildCPUDeviceCommandLine(cmd, def->cpus[i], qemuCaps) < 0) + goto error; + } + if (def->nvram) { if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2898876..ab161b1 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -92,6 +92,11 @@ qemuBuildChrDeviceStr(char **deviceStr, virDomainChrDefPtr chr, virQEMUCapsPtr qemuCaps); +int +qemuBuildCPUDeviceStr(char **deviceStr, + virDomainCPUDefPtr cpu, + virQEMUCapsPtr qemuCaps); + /* With vlan == -1, use netdev syntax, else old hostnet */ char *qemuBuildHostNetStr(virDomainNetDefPtr net, virQEMUDriverPtr driver, -- 1.9.3
On Wed, Jan 21, 2015 at 16:01:00 +0800, Zhu Guihua wrote:
qemuBuildCPUDeviceStr being introduced is responsible for creating command line argument for '-device' for given cpu device.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_command.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 5 +++++ 2 files changed, 55 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2ee3e10..824ad29 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5956,6 +5956,50 @@ static char *qemuBuildTPMDevStr(const virDomainDef *def, }
+int +qemuBuildCPUDeviceStr(char **deviceStr, + virDomainCPUDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU64_CPU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s not supported in this QEMU binary"), dev->driver);
You blindly assume that the user passed the correct model here and not any other invalid string. As I've already said, having this converted to an enum makes things easier.
+ goto error; + } + + virBufferAsprintf(&buf, "%s,id=%s,apic-id=%d", + dev->driver, dev->info.alias, + dev->apic_id);
Having a buffer for a single virBufferAsprintf case is a bit overkill.
+ + if (virBufferCheckError(&buf) < 0) + goto error; + + *deviceStr = virBufferContentAndReset(&buf); + return 0; + + error: + virBufferFreeAndReset(&buf); + return -1; +} + +static int +qemuBuildCPUDeviceCommandLine(virCommandPtr cmd, + virDomainCPUDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + char *devstr = NULL; + + if (qemuBuildCPUDeviceStr(&devstr, dev, qemuCaps) < 0) + return -1; + + virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); + return 0; +} + + static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -9862,6 +9906,12 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ /* add cpu devices */ + for (i = 0; i < def->ncpus; i++) { + if (qemuBuildCPUDeviceCommandLine(cmd, def->cpus[i], qemuCaps) < 0) + goto error; + }
As I've asked before. The question here is whether this is equivalent with just increasing the vCPU count in the "-cpu" argument. If not it will require a bit more work for setting cgroups, numa pinning and other stuff. If they are equivalent we should use that instead and have the existing code do the magic. Peter
On Wed, 2015-01-21 at 10:24 +0100, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 16:01:00 +0800, Zhu Guihua wrote:
qemuBuildCPUDeviceStr being introduced is responsible for creating command line argument for '-device' for given cpu device.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_command.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 5 +++++ 2 files changed, 55 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2ee3e10..824ad29 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5956,6 +5956,50 @@ static char *qemuBuildTPMDevStr(const virDomainDef *def, }
+int +qemuBuildCPUDeviceStr(char **deviceStr, + virDomainCPUDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU64_CPU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s not supported in this QEMU binary"), dev->driver);
You blindly assume that the user passed the correct model here and not any other invalid string. As I've already said, having this converted to an enum makes things easier.
+ goto error; + } + + virBufferAsprintf(&buf, "%s,id=%s,apic-id=%d", + dev->driver, dev->info.alias, + dev->apic_id);
Having a buffer for a single virBufferAsprintf case is a bit overkill.
+ + if (virBufferCheckError(&buf) < 0) + goto error; + + *deviceStr = virBufferContentAndReset(&buf); + return 0; + + error: + virBufferFreeAndReset(&buf); + return -1; +} + +static int +qemuBuildCPUDeviceCommandLine(virCommandPtr cmd, + virDomainCPUDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + char *devstr = NULL; + + if (qemuBuildCPUDeviceStr(&devstr, dev, qemuCaps) < 0) + return -1; + + virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); + return 0; +} + + static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -9862,6 +9906,12 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ /* add cpu devices */ + for (i = 0; i < def->ncpus; i++) { + if (qemuBuildCPUDeviceCommandLine(cmd, def->cpus[i], qemuCaps) < 0) + goto error; + }
As I've asked before. The question here is whether this is equivalent with just increasing the vCPU count in the "-cpu" argument.
If not it will require a bit more work for setting cgroups, numa pinning and other stuff.
Yes, we should require more work. Regards, Zhu
If they are equivalent we should use that instead and have the existing code do the magic.
Peter
This patch implements live hotplug of a cpu device. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 7 +++++++ 3 files changed, 68 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a88f6b4..ddc7eeb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6995,6 +6995,12 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break; case VIR_DOMAIN_DEVICE_CPU: + ret = qemuDomainAttachCPUDevice(driver, vm, + dev->data.cpu); + if (!ret) + dev->data.cpu = NULL; + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f6d7667..bff0d14 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1541,6 +1541,61 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, return ret; } +int +qemuDomainCPUInsert(virDomainDefPtr vmdef, + virDomainCPUDefPtr cpu) +{ + if (virDomainCPUFind(vmdef, cpu)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cpu already exists")); + return -1; + } + + if (virDomainCPUInsert(vmdef, cpu) < 0) + return -1; + + return 0; +} + +int qemuDomainAttachCPUDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainCPUDefPtr cpu) +{ + int ret = -1; + char *devstr = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + goto cleanup;; + } + + if (qemuAssignDeviceCPUAlias(vmdef, cpu, -1) < 0) + goto cleanup; + + if (qemuBuildCPUDeviceStr(&devstr, cpu, priv->qemuCaps) < 0) + goto cleanup; + + if (qemuDomainCPUInsert(vmdef, cpu) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + ignore_value(virBitmapSetBit(vm->def->apic_id_map, cpu->apic_id)); + ret = 0; + + cleanup: + VIR_FREE(devstr); + return ret; +} + static int qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 19ab9a0..6cdead3 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -106,6 +106,13 @@ qemuDomainChrInsert(virDomainDefPtr vmdef, virDomainChrDefPtr qemuDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr); +int +qemuDomainAttachCPUDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainCPUDefPtr cpu); +int +qemuDomainCPUInsert(virDomainDefPtr vmdef, + virDomainCPUDefPtr cpu); int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, -- 1.9.3
On Wed, Jan 21, 2015 at 16:01:01 +0800, Zhu Guihua wrote:
This patch implements live hotplug of a cpu device.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 7 +++++++ 3 files changed, 68 insertions(+)
...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f6d7667..bff0d14 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1541,6 +1541,61 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, return ret; }
+int +qemuDomainCPUInsert(virDomainDefPtr vmdef, + virDomainCPUDefPtr cpu) +{ + if (virDomainCPUFind(vmdef, cpu)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cpu already exists")); + return -1; + } + + if (virDomainCPUInsert(vmdef, cpu) < 0) + return -1; + + return 0; +} + +int qemuDomainAttachCPUDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainCPUDefPtr cpu) +{ + int ret = -1; + char *devstr = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + goto cleanup;; + } + + if (qemuAssignDeviceCPUAlias(vmdef, cpu, -1) < 0) + goto cleanup; + + if (qemuBuildCPUDeviceStr(&devstr, cpu, priv->qemuCaps) < 0) + goto cleanup; + + if (qemuDomainCPUInsert(vmdef, cpu) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm);
This function recently changed it's prototype and requires the return value to be checked. Rebasing to upstream will break build.
+ + ignore_value(virBitmapSetBit(vm->def->apic_id_map, cpu->apic_id)); + ret = 0; + + cleanup: + VIR_FREE(devstr); + return ret; +} + static int qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, virDomainObjPtr vm,a
Peter
On Wed, 2015-01-21 at 10:27 +0100, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 16:01:01 +0800, Zhu Guihua wrote:
This patch implements live hotplug of a cpu device.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 7 +++++++ 3 files changed, 68 insertions(+)
...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f6d7667..bff0d14 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1541,6 +1541,61 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, return ret; }
+int +qemuDomainCPUInsert(virDomainDefPtr vmdef, + virDomainCPUDefPtr cpu) +{ + if (virDomainCPUFind(vmdef, cpu)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cpu already exists")); + return -1; + } + + if (virDomainCPUInsert(vmdef, cpu) < 0) + return -1; + + return 0; +} + +int qemuDomainAttachCPUDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainCPUDefPtr cpu) +{ + int ret = -1; + char *devstr = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + goto cleanup;; + } + + if (qemuAssignDeviceCPUAlias(vmdef, cpu, -1) < 0) + goto cleanup; + + if (qemuBuildCPUDeviceStr(&devstr, cpu, priv->qemuCaps) < 0) + goto cleanup; + + if (qemuDomainCPUInsert(vmdef, cpu) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm);
This function recently changed it's prototype and requires the return value to be checked. Rebasing to upstream will break build.
I will confirm it, thanks. Regards, Zhu
+ + ignore_value(virBitmapSetBit(vm->def->apic_id_map, cpu->apic_id)); + ret = 0; + + cleanup: + VIR_FREE(devstr); + return ret; +} + static int qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, virDomainObjPtr vm,a
Peter
This patch implements live hotunplug of a cpu device. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 2 ++ src/qemu/qemu_hotplug.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 4 +++ 3 files changed, 72 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ddc7eeb..004bc35 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7077,6 +7077,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); break; case VIR_DOMAIN_DEVICE_CPU: + ret = qemuDomainDetachCPUDevice(driver, vm, dev->data.cpu); + break; case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bff0d14..41013d9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2876,6 +2876,20 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, static int +qemuDomainRemoveCPUDevice(virDomainObjPtr vm, + virDomainCPUDefPtr cpu) +{ + VIR_DEBUG("Removing cpu device %s from domain %p %s", + cpu->info.alias, vm, vm->def->name); + + virDomainCPURemove(vm->def, cpu); + virDomainCPUDefFree(cpu); + + return 0; +} + + +static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr) @@ -2941,6 +2955,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, break; case VIR_DOMAIN_DEVICE_CPU: + qemuDomainRemoveCPUDevice(vm, dev->data.cpu); + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: @@ -3841,3 +3858,52 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, VIR_FREE(devstr); return ret; } + +int qemuDomainDetachCPUDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainCPUDefPtr cpu) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + virDomainCPUDefPtr tmpCPU; + int rc = 0; + + if (!(tmpCPU = virDomainCPUFind(vmdef, cpu))) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configration")); + return ret; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + } + + if (!tmpCPU->info.alias) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("detaching cpu device without id is not supported")); + return ret; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmpCPU->info); + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDelDevice(priv->mon, tmpCPU->info.alias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + ignore_value(virBitmapClearBit(vm->def->apic_id_map, tmpCPU->apic_id)); + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveCPUDevice(vm, tmpCPU); + else + goto cleanup; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 6cdead3..5ec7b23 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -113,6 +113,10 @@ qemuDomainAttachCPUDevice(virQEMUDriverPtr driver, int qemuDomainCPUInsert(virDomainDefPtr vmdef, virDomainCPUDefPtr cpu); +int +qemuDomainDetachCPUDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainCPUDefPtr cpu); int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, -- 1.9.3
On Wed, Jan 21, 2015 at 16:01:02 +0800, Zhu Guihua wrote:
This patch implements live hotunplug of a cpu device.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 2 ++ src/qemu/qemu_hotplug.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 4 +++ 3 files changed, 72 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ddc7eeb..004bc35 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7077,6 +7077,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); break; case VIR_DOMAIN_DEVICE_CPU: + ret = qemuDomainDetachCPUDevice(driver, vm, dev->data.cpu); + break; case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bff0d14..41013d9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2876,6 +2876,20 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
static int +qemuDomainRemoveCPUDevice(virDomainObjPtr vm, + virDomainCPUDefPtr cpu) +{ + VIR_DEBUG("Removing cpu device %s from domain %p %s", + cpu->info.alias, vm, vm->def->name); + + virDomainCPURemove(vm->def, cpu); + virDomainCPUDefFree(cpu);
If cpu is not identical (in meaning of being the same pointer), just a definition denoting the same device this will leak the definition that was previously stored in vm->def.
+ + return 0; +} + + +static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr)
Peter
JSON array of cpu info is sorted in order to find thread id of cpu smoothly. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_monitor_json.c | 31 +++++++++++++++++++++++++++++-- src/util/virbitmap.c | 2 +- src/util/virbitmap.h | 2 ++ 4 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 032e9a9..d2c7c73 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1042,6 +1042,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapIsSet; virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index da5c14d..96a964c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1162,6 +1162,21 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon) return ret; } +static int +qemuCPUInfoCompare(const void *a, + const void *b) +{ + virJSONValuePtr *entrya = (virJSONValuePtr *)a; + virJSONValuePtr *entryb = (virJSONValuePtr *)b; + int ia; + int ib; + + virJSONValueObjectGetNumberInt(*entrya, "CPU", &ia); + virJSONValueObjectGetNumberInt(*entryb, "CPU", &ib); + + return ia - ib; +} + /* * [ { "CPU": 0, "current": true, "halted": false, "pc": 3227107138 }, @@ -1176,6 +1191,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, size_t i; int *threads = NULL; int ncpus; + virJSONValuePtr *entryarray = NULL; if (!(data = virJSONValueObjectGet(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1198,16 +1214,26 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, if (VIR_ALLOC_N(threads, ncpus) < 0) goto cleanup; + if (VIR_ALLOC_N(entryarray, ncpus) < 0) + goto cleanup; + for (i = 0; i < ncpus; i++) { virJSONValuePtr entry = virJSONValueArrayGet(data, i); - int thread; if (!entry) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cpu information was missing an array element")); goto cleanup; } - if (virJSONValueObjectGetNumberInt(entry, "thread_id", &thread) < 0) { + entryarray[i] = entry; + } + + qsort(entryarray, ncpus, sizeof(entryarray[0]), qemuCPUInfoCompare); + + for (i = 0; i < ncpus; i++) { + int thread; + if (virJSONValueObjectGetNumberInt(entryarray[i], "thread_id", &thread) < 0) { + /* Some older qemu versions don't report the thread_id, * so treat this as non-fatal, simply returning no data */ ret = 0; @@ -1223,6 +1249,7 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, cleanup: VIR_FREE(threads); + VIR_FREE(entryarray); return ret; } diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 05c50e4..168b8db 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -153,7 +153,7 @@ int virBitmapClearBit(virBitmapPtr bitmap, size_t b) } /* Helper function. caller must ensure b < bitmap->max_bit */ -static bool virBitmapIsSet(virBitmapPtr bitmap, size_t b) +bool virBitmapIsSet(virBitmapPtr bitmap, size_t b) { return !!(bitmap->map[VIR_BITMAP_UNIT_OFFSET(b)] & VIR_BITMAP_BIT(b)); } diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 565264c..57fb195 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -60,6 +60,8 @@ int virBitmapSetBit(virBitmapPtr bitmap, size_t b) int virBitmapClearBit(virBitmapPtr bitmap, size_t b) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +bool virBitmapIsSet(virBitmapPtr bitmap, size_t b); + /* * Get setting of bit position @b in @bitmap and store in @result */ -- 1.9.3
On Wed, Jan 21, 2015 at 16:01:03 +0800, Zhu Guihua wrote:
JSON array of cpu info is sorted in order to find thread id of cpu smoothly.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_monitor_json.c | 31 +++++++++++++++++++++++++++++-- src/util/virbitmap.c | 2 +- src/util/virbitmap.h | 2 ++ 4 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 032e9a9..d2c7c73 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1042,6 +1042,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapIsSet;
This function is not used anywhere in this patch. Is this (and the other related hunks) here by accident?
virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy;
Peter
On Wed, 2015-01-21 at 10:34 +0100, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 16:01:03 +0800, Zhu Guihua wrote:
JSON array of cpu info is sorted in order to find thread id of cpu smoothly.
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_monitor_json.c | 31 +++++++++++++++++++++++++++++-- src/util/virbitmap.c | 2 +- src/util/virbitmap.h | 2 ++ 4 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 032e9a9..d2c7c73 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1042,6 +1042,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapIsSet;
This function is not used anywhere in this patch. Is this (and the other related hunks) here by accident?
It is an accident, it should be in another patch. Thanks, Zhu
virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy;
Peter
After hotplugging the CPUs, we need to re-detect threads corresponding to Vcpus. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 271 ++++++++++++++++++++++++------------------------ src/qemu/qemu_driver.h | 8 ++ src/qemu/qemu_hotplug.c | 7 ++ 3 files changed, 152 insertions(+), 134 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 004bc35..09ac088 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4357,89 +4357,49 @@ static void qemuProcessEventHandler(void *data, void *opaque) VIR_FREE(processEvent); } -static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, - virDomainObjPtr vm, - unsigned int nvcpus) +/* After hotplugging the CPUs we need to re-detect threads corresponding + * * to the virtual CPUs. Some older versions don't provide the thread ID + * * or don't have the "info cpus" command (and they don't support multiple + * * CPUs anyways), so errors in the re-detection will not be treated + * * fatal */ +int +qemuDomainDetectVcpu(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int apic_id, + bool plug) { qemuDomainObjPrivatePtr priv = vm->privateData; - size_t i; - int rc = 1; - int ret = -1; int oldvcpus = vm->def->vcpus; int vcpus = oldvcpus; pid_t *cpupids = NULL; int ncpupids; + int ret = -1; virCgroupPtr cgroup_vcpu = NULL; char *mem_mask = NULL; - uint32_t apic_id; - - qemuDomainObjEnterMonitor(driver, vm); - - /* We need different branches here, because we want to offline - * in reverse order to onlining, so any partial fail leaves us in a - * reasonably sensible state */ - if (nvcpus > vcpus) { - for (i = vcpus; i < nvcpus; i++) { - /* Online new CPU */ - apic_id = virDomainCPUGetFreeApicID(vm->def); - rc = qemuMonitorSetCPU(priv->mon, apic_id, true); - if (rc == 0) - goto unsupported; - if (rc < 0) - goto exit_monitor; - - vcpus++; - ignore_value(virBitmapSetBit(vm->def->apic_id_map, apic_id)); - } - } else { - for (i = vcpus - 1; i >= nvcpus; i--) { - /* Offline old CPU */ - rc = qemuMonitorSetCPU(priv->mon, i, false); - if (rc == 0) - goto unsupported; - if (rc < 0) - goto exit_monitor; - - vcpus--; - } - } - - /* hotplug succeeded */ + int idx = 0; + size_t i; + int *thread = NULL; ret = 0; + if (plug) + vcpus++; + else + vcpus--; + + qemuDomainObjEnterMonitor(driver, vm); - /* After hotplugging the CPUs we need to re-detect threads corresponding - * to the virtual CPUs. Some older versions don't provide the thread ID - * or don't have the "info cpus" command (and they don't support multiple - * CPUs anyways), so errors in the re-detection will not be treated - * fatal */ - if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) { + if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0){ virResetLastError(); - goto exit_monitor; - } - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - ret = -1; goto cleanup; } - /* check if hotplug has failed */ - if (vcpus < oldvcpus && ncpupids == oldvcpus) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("qemu didn't unplug the vCPUs properly")); - vcpus = oldvcpus; - ret = -1; - goto cleanup; + for (i = 0; i < apic_id; i++) { + if (virBitmapIsSet(vm->def->apic_id_map, i)) + idx++; } - if (ncpupids != vcpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("got wrong number of vCPU pids from QEMU monitor. " - "got %d, wanted %d"), - ncpupids, vcpus); - vcpus = oldvcpus; - ret = -1; + if (VIR_ALLOC_N(thread, vcpus) < 0) goto cleanup; - } if (virDomainNumatuneGetMode(vm->def->numatune, -1) == VIR_DOMAIN_NUMATUNE_MEM_STRICT && @@ -4448,105 +4408,148 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, &mem_mask, -1) < 0) goto cleanup; - if (nvcpus > oldvcpus) { - for (i = oldvcpus; i < nvcpus; i++) { - if (priv->cgroup) { - int rv = -1; - /* Create cgroup for the onlined vcpu */ - if (virCgroupNewVcpu(priv->cgroup, i, true, &cgroup_vcpu) < 0) - goto cleanup; - - if (mem_mask && - virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0) - goto cleanup; + if (vcpus > oldvcpus) { + if (priv->cgroup) { + int rv = -1; + if (virCgroupNewVcpu(priv->cgroup, apic_id, true, &cgroup_vcpu) < 0) + goto cleanup; - /* Add vcpu thread to the cgroup */ - rv = virCgroupAddTask(cgroup_vcpu, cpupids[i]); - if (rv < 0) { - virReportSystemError(-rv, - _("unable to add vcpu %zu task %d to cgroup"), - i, cpupids[i]); - virCgroupRemove(cgroup_vcpu); - goto cleanup; - } + if (mem_mask && + virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0) + goto cleanup; + rv = virCgroupAddTask(cgroup_vcpu, cpupids[idx]); + if (rv < 0) { + virReportSystemError(-rv, + _("unable to add vcpu %d task %d to cgroup"), + apic_id, cpupids[idx]); + virCgroupRemove(cgroup_vcpu); + goto cleanup; } + } - /* Inherit def->cpuset */ - if (vm->def->cpumask) { - /* vm->def->cputune.vcpupin can't be NULL if - * vm->def->cpumask is not NULL. - */ - virDomainVcpuPinDefPtr vcpupin = NULL; + if (vm->def->cpumask) { + virDomainVcpuPinDefPtr vcpupin = NULL; - if (VIR_ALLOC(vcpupin) < 0) - goto cleanup; + if (VIR_ALLOC(vcpupin) < 0) + goto cleanup; - vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); - virBitmapCopy(vcpupin->cpumask, vm->def->cpumask); - vcpupin->vcpuid = i; - if (VIR_APPEND_ELEMENT_COPY(vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin, vcpupin) < 0) { - virBitmapFree(vcpupin->cpumask); - VIR_FREE(vcpupin); + vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); + virBitmapCopy(vcpupin->cpumask, vm->def->cpumask); + vcpupin->vcpuid = apic_id; + if (VIR_APPEND_ELEMENT_COPY(vm->def->cputune.vcpupin, + vm->def->cputune.nvcpupin, vcpupin) < 0) { + virBitmapFree(vcpupin->cpumask); + VIR_FREE(vcpupin); + ret = -1; + goto cleanup; + } + + if (cgroup_vcpu) { + if (qemuSetupCgroupVcpuPin(cgroup_vcpu, + vm->def->cputune.vcpupin, + vm->def->cputune.nvcpupin, apic_id) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("failed to set cpuset.cpus in cgroup" + " for vcpu %d"), apic_id); ret = -1; goto cleanup; } - - if (cgroup_vcpu) { - if (qemuSetupCgroupVcpuPin(cgroup_vcpu, - vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin, i) < 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("failed to set cpuset.cpus in cgroup" - " for vcpu %zu"), i); - ret = -1; - goto cleanup; - } - } else { - if (virProcessSetAffinity(cpupids[i], - vcpupin->cpumask) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("failed to set cpu affinity for vcpu %zu"), - i); - ret = -1; - goto cleanup; - } + } else { + if (virProcessSetAffinity(cpupids[idx], + vcpupin->cpumask) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to set cpu affinity for vcpu %d"), + apic_id); + ret = -1; + goto cleanup; } } virCgroupFree(&cgroup_vcpu); } } else { - for (i = oldvcpus - 1; i >= nvcpus; i--) { - if (priv->cgroup) { - if (virCgroupNewVcpu(priv->cgroup, i, false, &cgroup_vcpu) < 0) - goto cleanup; - - /* Remove cgroup for the offlined vcpu */ - virCgroupRemove(cgroup_vcpu); - virCgroupFree(&cgroup_vcpu); - } + if (priv->cgroup) { + if (virCgroupNewVcpu(priv->cgroup, apic_id, false, &cgroup_vcpu) < 0) + goto cleanup; - /* Free vcpupin setting */ - virDomainVcpuPinDel(vm->def, i); + virCgroupRemove(cgroup_vcpu); + virCgroupFree(&cgroup_vcpu); } + + virDomainVcpuPinDel(vm->def, apic_id); } priv->nvcpupids = ncpupids; VIR_FREE(priv->vcpupids); priv->vcpupids = cpupids; cpupids = NULL; + thread = NULL; cleanup: + qemuDomainObjExitMonitor(driver, vm); + vm->def->vcpus = vcpus; VIR_FREE(cpupids); + VIR_FREE(thread); VIR_FREE(mem_mask); - if (virDomainObjIsActive(vm)) - vm->def->vcpus = vcpus; - virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1); + virDomainAuditVcpu(vm, oldvcpus, vcpus, "update", true); if (cgroup_vcpu) virCgroupFree(&cgroup_vcpu); return ret; +} + +static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int nvcpus) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + size_t i; + int rc = 1; + int ret = -1; + int oldvcpus = vm->def->vcpus; + int vcpus = oldvcpus; + uint32_t apic_id; + + /* We need different branches here, because we want to offline + * in reverse order to onlining, so any partial fail leaves us in a + * reasonably sensible state */ + if (nvcpus > vcpus) { + for (i = vcpus; i < nvcpus; i++) { + /* Online new CPU */ + qemuDomainObjEnterMonitor(driver, vm); + apic_id = virDomainCPUGetFreeApicID(vm->def); + rc = qemuMonitorSetCPU(priv->mon, apic_id, true); + if (rc == 0) + goto unsupported; + if (rc < 0) + goto exit_monitor; + + qemuDomainObjExitMonitor(driver, vm); + vcpus++; + ignore_value(virBitmapSetBit(vm->def->apic_id_map, apic_id)); + + if (qemuDomainDetectVcpu(driver, vm, apic_id, true) < 0) + goto cleanup; + } + } else { + for (i = vcpus - 1; i >= nvcpus; i--) { + /* Offline old CPU */ + rc = qemuMonitorSetCPU(priv->mon, i, false); + if (rc == 0) + goto unsupported; + if (rc < 0) + goto exit_monitor; + + vcpus--; + } + } + + /* hotplug succeeded */ + + ret = 0; + + cleanup: + return ret; unsupported: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/qemu/qemu_driver.h b/src/qemu/qemu_driver.h index df7533a..55ce618 100644 --- a/src/qemu/qemu_driver.h +++ b/src/qemu/qemu_driver.h @@ -20,10 +20,18 @@ * * Author: Daniel P. Berrange <berrange@redhat.com> */ +#include "qemu_conf.h" +#include "domain_conf.h" #ifndef __QEMU_DRIVER_H__ # define __QEMU_DRIVER_H__ int qemuRegister(void); +int +qemuDomainDetectVcpu(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int apic_id, + bool plug); + #endif /* __QEMU_DRIVER_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 41013d9..8a20304 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -31,6 +31,7 @@ #include "qemu_command.h" #include "qemu_hostdev.h" #include "qemu_interface.h" +#include "qemu_driver.h" #include "domain_audit.h" #include "netdev_bandwidth_conf.h" #include "domain_nwfilter.h" @@ -1591,6 +1592,9 @@ int qemuDomainAttachCPUDevice(virQEMUDriverPtr driver, ignore_value(virBitmapSetBit(vm->def->apic_id_map, cpu->apic_id)); ret = 0; + if (qemuDomainDetectVcpu(driver, vm, cpu->apic_id, true) < 0) + ret = -1; + cleanup: VIR_FREE(devstr); return ret; @@ -3903,6 +3907,9 @@ int qemuDomainDetachCPUDevice(virQEMUDriverPtr driver, else goto cleanup; + if (qemuDomainDetectVcpu(driver, vm, tmpCPU->apic_id, false) < 0) + ret = -1; + cleanup: qemuDomainResetDeviceRemoval(vm); return ret; -- 1.9.3
On Wed, Jan 21, 2015 at 16:00:52 +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
The cpu device's xml like this: <cpu driver='qemu64-x86_64-cpu' apic_id='3'>
Libvirt already implements vCPU hotplug and unplug via virDomainSetVcpusFlags() API and implements this API also for the qemu driver. Granted that the existing code uses older comands but that can be tweaked to support the new stuff too. Additionally the new <cpu> subelement of <devices> doesn't seem necessary: - There's only one model that can be plugged - Configuring the APIC id doesn't make much sense for users - nothing else can be configured I have following questions though: Is there a difference in specifying the cpu via the -device option and via the legacy -cpu argument? Is there a plan to have more than one cpu "model" that can be plugged? Can more than one cpu model be plugged to the same VM at the same time? In general I don't think it's necessary to add the new device type and we should rather improve the existing API to support the new device type. More comments will be inline in the patches. Peter
On Wed, 2015-01-21 at 09:49 +0100, Peter Krempa wrote:
On Wed, Jan 21, 2015 at 16:00:52 +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
The cpu device's xml like this: <cpu driver='qemu64-x86_64-cpu' apic_id='3'>
Libvirt already implements vCPU hotplug and unplug via virDomainSetVcpusFlags() API and implements this API also for the qemu driver. Granted that the existing code uses older comands but that can be tweaked to support the new stuff too.
Now Libvirt only supported vCPU hotplug by qemu command 'cpu-add', but vCPU hot-unplug has not been implemented for qemu driver.
Additionally the new <cpu> subelement of <devices> doesn't seem necessary:
- There's only one model that can be plugged
there are more model will be plugged in next version.
- Configuring the APIC id doesn't make much sense for users
I agree. I will not consider the APIC id in future.
- nothing else can be configured
Yes, only cpu model shoud be configured.
I have following questions though:
Is there a difference in specifying the cpu via the -device option and via the legacy -cpu argument?
Of course, -device has more functions than -cpu.
Is there a plan to have more than one cpu "model" that can be plugged?
Yes, all x86 cpu model will be supported. This is an RFC patchset, so the feature is realized in a simple way.
Can more than one cpu model be plugged to the same VM at the same time?
Yes, it can.
In general I don't think it's necessary to add the new device type and we should rather improve the existing API to support the new device type.
I think it's necessary to support more cpu model. When user hotplug a cpu, it is essential to point the cpu model. And I think improve the existing API would change too much. The feature is implemented by command 'device_add', so I think it should also keep consistent with the other devices' hotplug. Regards, Zhu
More comments will be inline in the patches.
Peter
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
The cpu device's xml like this: <cpu driver='qemu64-x86_64-cpu' apic_id='3'>
Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. It feels like a rather low level QEMU private implementation detail to me that apps should not need to know or care about. I think libvirt should always just do the right thing to make cpu hotplug work. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, 2015-01-21 at 09:42 +0000, Daniel P. Berrange wrote:
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
The cpu device's xml like this: <cpu driver='qemu64-x86_64-cpu' apic_id='3'>
Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. It feels like a rather low level QEMU private implementation detail to me that apps should not need to know or care about. I think libvirt should always just do the right thing to make cpu hotplug work.
There is a need to use more cpu model. 'qemu64-x86_64-cpu' is only one example, we will realize more driver in future. Regards, Zhu
Regards, Daniel
On Thu, Jan 22, 2015 at 04:55:02PM +0800, Zhu Guihua wrote:
On Wed, 2015-01-21 at 09:42 +0000, Daniel P. Berrange wrote:
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
The cpu device's xml like this: <cpu driver='qemu64-x86_64-cpu' apic_id='3'>
Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. It feels like a rather low level QEMU private implementation detail to me that apps should not need to know or care about. I think libvirt should always just do the right thing to make cpu hotplug work.
There is a need to use more cpu model. 'qemu64-x86_64-cpu' is only one example, we will realize more driver in future.
Can you give an example of why we will need more than one model ? It seems pretty crazy to me that we will need to specify two CPU models for CPUs, both "Nehalem" / "Opteron" / etc and this new CPU model. It makes little sense from the user / app POV IMHO. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, 2015-01-22 at 10:06 +0000, Daniel P. Berrange wrote:
On Thu, Jan 22, 2015 at 04:55:02PM +0800, Zhu Guihua wrote:
On Wed, 2015-01-21 at 09:42 +0000, Daniel P. Berrange wrote:
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
The cpu device's xml like this: <cpu driver='qemu64-x86_64-cpu' apic_id='3'>
Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. It feels like a rather low level QEMU private implementation detail to me that apps should not need to know or care about. I think libvirt should always just do the right thing to make cpu hotplug work.
There is a need to use more cpu model. 'qemu64-x86_64-cpu' is only one example, we will realize more driver in future.
Can you give an example of why we will need more than one model ? It seems pretty crazy to me that we will need to specify two CPU models for CPUs, both "Nehalem" / "Opteron" / etc and this new CPU model. It makes little sense from the user / app POV IMHO.
In future, this will become an advanced feature for specific user. If user may not specific a model, we will specific a default model. I think this feature is mainly for test environment. If you want to test different cpu model's performance, this feature will be essential. Regards, Zhu
Regards, Daniel
On Thu, Jan 22, 2015 at 07:02:27PM +0800, Zhu Guihua wrote:
On Thu, 2015-01-22 at 10:06 +0000, Daniel P. Berrange wrote:
On Thu, Jan 22, 2015 at 04:55:02PM +0800, Zhu Guihua wrote:
On Wed, 2015-01-21 at 09:42 +0000, Daniel P. Berrange wrote:
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
The cpu device's xml like this: <cpu driver='qemu64-x86_64-cpu' apic_id='3'>
Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. It feels like a rather low level QEMU private implementation detail to me that apps should not need to know or care about. I think libvirt should always just do the right thing to make cpu hotplug work.
There is a need to use more cpu model. 'qemu64-x86_64-cpu' is only one example, we will realize more driver in future.
Can you give an example of why we will need more than one model ? It seems pretty crazy to me that we will need to specify two CPU models for CPUs, both "Nehalem" / "Opteron" / etc and this new CPU model. It makes little sense from the user / app POV IMHO.
In future, this will become an advanced feature for specific user. If user may not specific a model, we will specific a default model.
I think this feature is mainly for test environment. If you want to test different cpu model's performance, this feature will be essential.
The choice of Nehalem, Opteron, etc as CPU models is already supported in QEMU and influences guest CPU performance. You're not explaining why we need to introduce multiple CPU <cpu driver='qemu64-x86_64-cpu'> values. It makes no sense to have two different CPU models listed for the same guest like this. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
[...]
The choice of Nehalem, Opteron, etc as CPU models is already supported in QEMU and influences guest CPU performance. You're not explaining why we need to introduce multiple CPU <cpu driver='qemu64-x86_64-cpu'> values. It makes no sense to have two different CPU models listed for the same guest like this.
Hi Daniel, Maybe I misunderstood your meaning. Next I will let you know what we think in detail. libvirt already implements Vcpu hotplug for qemu driver by qemu command 'cpu-add', but Vcpu hot-unplug has not been implemented. qemu prefers to implement Vcpu hotplug by command 'device_add' instead of 'cpu-add', implement Vcpu hotplug by command 'device_del' in future. So, according to the above, our team has the following two methods to implement Vcpu hotplug/un-plug in libvirt. 1) improve the existing API, and use the old command 'setVcpus'. This method will invoke qemu's command 'device_add' instead of 'cpu-add'. This method has three problems: a) can not specific the cpu' model. This will change qemu's design for cpu hotplug. b) can not keep consistent with other devices' hotplug in libvirt. c) influence cpu's hot-unplug. If device_add a cpu without id (i.e. device alias name in libvirt), cpu hot-unplug can not work. 2) Add a new API to support cpu hot-plug/unplug, and leave the the existing API as a legacy. This method will realize the feature by libvirt command 'attach-device', and will allow user to specify the cpu model. And in the future, we want to make struct virCPUDefPtr as a member of the new defined struct for hotplugged cpu device, so that We can allow user to set more details for cpu device, not only cpu model. This method has two advantages. a) keep consistent with other devices' hotplug in libvirt. b) can assign device alias name. This is helpful for cpu hot-unplug. The above is our opinion. May you give us some suggestions? Regards, Zhu
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
Also I'm wondering how this interacts with CPU topology. eg lets say you configure a 16 vCPU guest, and set topology to 2 sockets, 4 cores, 2 threads. Does this hotplug allow you to plug/unplug individual threads - ie each invididual vCPU, or does it only allow plug/unplug of sockets - ie 8 vCPUs at a time in this topology. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, 2015-01-21 at 09:57 +0000, Daniel P. Berrange wrote:
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
Also I'm wondering how this interacts with CPU topology. eg lets say you configure a 16 vCPU guest, and set topology to 2 sockets, 4 cores, 2 threads. Does this hotplug allow you to plug/unplug individual threads - ie each invididual vCPU, or does it only allow plug/unplug of sockets - ie 8 vCPUs at a time in this topology.
qemu has not interacted with CPU topology so for. So now we focus on this feature's realization in a simple way. Regards, Zhu
Regards, Daniel
On Thu, Jan 22, 2015 at 04:57:39PM +0800, Zhu Guihua wrote:
On Wed, 2015-01-21 at 09:57 +0000, Daniel P. Berrange wrote:
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
Also I'm wondering how this interacts with CPU topology. eg lets say you configure a 16 vCPU guest, and set topology to 2 sockets, 4 cores, 2 threads. Does this hotplug allow you to plug/unplug individual threads - ie each invididual vCPU, or does it only allow plug/unplug of sockets - ie 8 vCPUs at a time in this topology.
qemu has not interacted with CPU topology so for. So now we focus on this feature's realization in a simple way.
What do you mean by that ? eg which impl I describe above will it support ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, 2015-01-22 at 10:04 +0000, Daniel P. Berrange wrote:
On Thu, Jan 22, 2015 at 04:57:39PM +0800, Zhu Guihua wrote:
On Wed, 2015-01-21 at 09:57 +0000, Daniel P. Berrange wrote:
On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote:
If you apply the folowing patchset [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, and [PATCH v2 00/11] cpu: add i386 cpu hot remove support https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, qemu can support hotplug and hot-unplug cpu device.
So this patch series will make libvirt support hotplug and hot-unplug cpu device for qemu driver, and now only supports one cpu driver which is 'qemu64-x86_64-cpu'.
Also I'm wondering how this interacts with CPU topology. eg lets say you configure a 16 vCPU guest, and set topology to 2 sockets, 4 cores, 2 threads. Does this hotplug allow you to plug/unplug individual threads - ie each invididual vCPU, or does it only allow plug/unplug of sockets - ie 8 vCPUs at a time in this topology.
qemu has not interacted with CPU topology so for. So now we focus on this feature's realization in a simple way.
What do you mean by that ? eg which impl I describe above will it support ?
Only allow plug/unplug of sockets. Regards, Zhu
Regards, Daniel
participants (3)
-
Daniel P. Berrange -
Peter Krempa -
Zhu Guihua